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

Suggest valid crate type if invalid crate type is found #54173

Merged
merged 1 commit into from Sep 14, 2018

Conversation

@phansch
Copy link
Member

@phansch phansch commented Sep 13, 2018

This adds a suggestion to the invalid_crate_types lint.

The suggestion is based on the Levenshtein distance to existing crate
types. If no suggestion is found it will show the lint without any
suggestions.

Closes #53958

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Sep 13, 2018

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@phansch
Copy link
Member Author

@phansch phansch commented Sep 13, 2018

Should this be a machine applicable suggestion? I'm not really sure if the Levenshtein candidate is good enough in this case because cdylib, dylib, rlib and lib are pretty close to each other.

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Sep 13, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:38] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:39] tidy error: /checkout/src/librustc_driver/driver.rs:1525: line longer than 100 chars
[00:04:39] tidy error: /checkout/src/librustc_driver/driver.rs:1533: line longer than 100 chars
[00:04:40] some tidy checks failed
[00:04:40] 
[00:04:40] 
[00:04:40] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:40] 
[00:04:40] 
[00:04:40] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:40] Build completed unsuccessfully in 0:00:49
[00:04:40] Build completed unsuccessfully in 0:00:49
[00:04:40] make: *** [tidy] Error 1
[00:04:40] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0a75dbec
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:01901004:start=1536822065529320655,finish=1536822065534486284,duration=5165629
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:112fd12a
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02bfb2e2
travis_time:start:02bfb2e2
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:05e379e2
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@davidtwco
Copy link
Member

@davidtwco davidtwco commented Sep 13, 2018

Should this be a machine applicable suggestion? I'm not really sure if the Levenshtein candidate is good enough in this case because cdylib, dylib, rlib and lib are pretty close to each other.

I think Applicability::MaybeIncorrect is a good choice here, the suggestion is likely correct but it might not be - this seems similar to other uses I can find of Applicability::MaybeIncorrect.


Otherwise, this looks good to me. Assigning someone else to take another look who might be more familiar with this part of the code.

r? @oli-obk (based on github's suggestions)

@rust-highfive rust-highfive assigned oli-obk and unassigned davidtwco Sep 13, 2018
@phansch
Copy link
Member Author

@phansch phansch commented Sep 13, 2018

I think @oli-obk is on holiday until October, so maybe.. r? @estebank
(will fix tidy + machine applicable suggestion later today)

@rust-highfive rust-highfive assigned estebank and unassigned oli-obk Sep 13, 2018
Copy link
Contributor

@estebank estebank left a comment

Minor changes requested. r=me after changing.

--> $DIR/invalid-crate-type.rs:16:15
|
LL | #![crate_type="statoclib"]
| ^^^^^^^^^^^ help: did you mean: `staticlib`

This comment has been minimized.

@estebank

estebank Sep 13, 2018
Contributor

Notice that the span includes the "s, but the suggestion doesn't, which would make applying this suggestion generate the code #![crate_type=staticlib], which is incorrect. Easiest solution is to include the quotes in the suggestion.

span,
"invalid `crate_type` value",
lint::builtin::BuiltinLintDiagnostics::
UnknownCrateTypes(span, "did you mean".to_string(), candidate.to_string())

This comment has been minimized.

@estebank

estebank Sep 13, 2018
Contributor

Change to

lint::builtin::BuiltinLintDiagnostics::UnknownCrateTypes(
    span,
    "did you mean".to_string(),
    format!("\"{}\"", candidate),
);
@@ -500,6 +501,9 @@ impl BuiltinLintDiagnostics {
Applicability::MachineApplicable
);
}
BuiltinLintDiagnostics::UnknownCrateTypes(span, note, sugg) => {
db.span_suggestion(span, &note, sugg);

This comment has been minimized.

@estebank

estebank Sep 13, 2018
Contributor

As noted in the prior discussion, use span_suggestion_with_applicability. MaybeIncorrect is always a safe bet to use, MachineApplicable is not consistently used throughout the codebase. It sometimes means 100% certainty, while others it means "95% of the cases this error fires, this suggestion is correct". I'm ok with the overly restrictive reading of the meaning.

];
if let ast::MetaItemKind::NameValue(spanned) = a.meta().unwrap().node {
let span = spanned.span;
let lev_candidate = find_best_match_for_name(crate_types.iter(), &n.as_str(), None);

This comment has been minimized.

@estebank

estebank Sep 13, 2018
Contributor

This line is too long, reformat.

This adds a suggestion to the `invalid_crate_types` lint.

The suggestion is based on the Levenshtein distance to existing crate
types. If no suggestion is found it will show the lint without any
suggestions.
@phansch phansch force-pushed the phansch:suggest_valid_crate_type branch from 8830670 to 7249a1b Sep 13, 2018
@phansch
Copy link
Member Author

@phansch phansch commented Sep 13, 2018

I sort of did a rebase by accident instead of adding new commits, but it should be all good now 👍

@estebank
Copy link
Contributor

@estebank estebank commented Sep 13, 2018

@bors r+ rollup

@bors
Copy link
Contributor

@bors bors commented Sep 13, 2018

📌 Commit 7249a1b has been approved by estebank

kennytm added a commit to kennytm/rust that referenced this pull request Sep 14, 2018
…=estebank

Suggest valid crate type if invalid crate type is found

This adds a suggestion to the `invalid_crate_types` lint.

The suggestion is based on the Levenshtein distance to existing crate
types. If no suggestion is found it will show the lint without any
suggestions.

Closes rust-lang#53958
bors added a commit that referenced this pull request Sep 14, 2018
Rollup of 8 pull requests

Successful merges:

 - #53218 (Add a implementation of `From` for converting `&'a Option<T>` into `Option<&'a T>`)
 - #54024 (Fix compiling some rustc crates to wasm)
 - #54095 (Rename all mentions of `nil` to `unit`)
 - #54173 (Suggest valid crate type if invalid crate type is found)
 - #54194 (Remove println!() statement from HashMap unit test)
 - #54203 (Fix the stable release of os_str_str_ref_eq)
 - #54207 (re-mark the never docs as unstable)
 - #54210 (Update Cargo)

Failed merges:

r? @ghost
@bors bors merged commit 7249a1b into rust-lang:master Sep 14, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@phansch phansch deleted the phansch:suggest_valid_crate_type branch Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.