Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint unnecessary conversion of OsString or CString to String #2259

Open
clarfonthey opened this issue Dec 5, 2017 · 3 comments
Open

Lint unnecessary conversion of OsString or CString to String #2259

clarfonthey opened this issue Dec 5, 2017 · 3 comments
Labels
E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Dec 5, 2017

For example, something along the lines of:

let files = Vec::new();
for arg in env::args() {
    files.push(File::open(arg));
}

should be suggested as:

let files = Vec::new();
for arg in env::args_os() {
    files.push(File::open(arg));
}

This would also include something like:

let files = Vec::new();
for arg in env::args() {
    files.push(File::open(arg.to_string_lossy()));
}
@oli-obk oli-obk added L-unnecessary Lint: Warn about unnecessary code E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-middle Type: Probably requires verifiying types labels Dec 5, 2017
@HMPerson1
Copy link
Contributor

HMPerson1 commented Jan 25, 2018

So it looks like std::env::args/var/vars are the only functions that have _os alternatives.

Unless I'm missing something, there's no easy way to programmatically find these, so this list has to be hard-coded into the lint, so it won't work with other crates that have pairs of functions like this.

@HMPerson1
Copy link
Contributor

Also what exactly constitutes 'unnecessary'? If we define it as 'anywhere where Str(ing) would also typecheck', then that would also catch whenever it's put into a generic data structure, and maybe that would be a false positive since we can't easily know how that data structure is going to be used later?

Or, since File::open/create both take T where T: AsRef<Path>, and both Str(ing) and OsStr(ing) implement that, perhaps we define unnecessary as 'passed as a parameter that is defined as taking T where T: [something] and both Str(ing) and Os/CStr(ing) implement [something]'? But then if that [something] is something like Hash, then that would still catch putting it into a data structure.

Maybe only for traits that are implemented only by Str(ing) and Os/CStr(ing), but that would exclude AsRef<Path> since that's also implemented by std::path::Components.

Or we could avoid this whole mess and just hard-code a list of places where we know for sure that converting to Str(ing) is unnecessary.

I don't know, I'm just thinking out loud.

@HMPerson1
Copy link
Contributor

Actually, I think defining 'unnecessary' as 'passed as a parameter where the return type doesn't change if Os/CStr(ing) were Str(ing) instead' would work well. Though, if there are associated types involved, that would require trait impl lookups, and I'm not sure how expensive that would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

3 participants