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

Use structured suggestions for nonstandard style lints #57387

Merged
merged 4 commits into from Jan 14, 2019

Conversation

Projects
None yet
6 participants
@euclio
Copy link
Contributor

euclio commented Jan 6, 2019

This PR modifies the lints in the nonstandard_style group to use structured suggestions. Note that there's a bit of tricky span calculation going on for the crate_name attribute. It also simplifies the code a bit: I don't think the "fallback" suggestions for these lints can actually be triggered.

Fixes #48103.
Fixes #52414.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 6, 2019

r? @davidtwco

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

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Jan 6, 2019

The job x86_64-gnu-llvm-6.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.
travis_time:end:02050ca4:start=1546812741754778094,finish=1546812742729666673,duration=974888579
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:59:03] 
[00:59:03] 30    |
[00:59:03] 31    = help: add #![feature(non_ascii_idents)] to the crate attributes to enable
[00:59:03] 32 
[00:59:03] - warning: type parameter `γ` should have a camel case name such as `Γ`
[00:59:03] + warning: type parameter `γ` should have a camel case name
[00:59:03] 35    |
[00:59:03] 35    |
[00:59:03] 36 LL |     γ  //~ ERROR non-ascii idents are not fully supported
[00:59:03] -    |     ^
[00:59:03] -    |     ^
[00:59:03] +    |     ^ help: convert the identifier to camel case: `Γ`
[00:59:03] 39    = note: #[warn(non_camel_case_types)] on by default
[00:59:03] 40 
[00:59:03] 
[00:59:03] 
[00:59:03] 
[00:59:03] The actual stderr differed from the expected stderr.
[00:59:03] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/utf8_idents/utf8_idents.stderr
[00:59:03] To update references, rerun the tests and pass the `--bless` flag
[00:59:03] To only update this specific test, also pass `--test-args utf8_idents.rs`
[00:59:03] error: 1 errors occurred comparing output.
[00:59:03] status: exit code: 1
[00:59:03] status: exit code: 1
[00:59:03] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/utf8_idents.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/utf8_idents/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/i/utf8_idents.rs:5:5\n   |\nLL |     γ  //~ ERROR non-ascii idents are not fully supported\n   |     ^\n   |\n   = help: add #![feature(non_ascii_idents)] to the crate attributes to enable\n\n"}
[00:59:03] {"message":"non-ascii idents are not fully supported. (see issue #55467)","code":{"code":"E0658","explanation":"\nAn unstable feature was used.\n\nErroneous code example:\n\n```compile_fail,E658\n#[repr(u128)] // error: use of unstable library feature 'repr128'\nenum Foo {\n    Bar(u64),\n}\n```\n\nIf you're using a stable or a beta version of rustc, you won't be able to use\nany unstable features. In order to do so, please switch to a nightly version of\nrustc (by using rustup).\n\nIf you're using a nightly version of rustc, just add the corresponding feature\nto be able to use it:\n\n```\n#![feature(repr128)]\n\n#[repr(u128)] // ok!\nenum Foo {\n    Bar(u64),\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/utf8_idents.rs","byte_start":234,"byte_end":236,"line_start":10,"line_end":10,"column_start":5,"column_end":6,"is_primary":true,"text":[{"text":"    δ: usize //~ ERROR non-ascii idents are not fully supported","highlight_start":5,"highlight_end":6}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"add #![feature(non_ascii_idents)] to the crate attributes to enable","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":"error[E0658]: non-ascii idents are not fully supported. (see issue #55467)\n  --> /checkout/src/test/ui/utf8_idents.rs:10:5\n   |\nLL |     δ: usize //~ ERROR non-ascii idents are nonux-gnu/stage2/lib/rustlib
46352 ./src/llvm-emscripten/test/MC
44784 ./src/test/ui
44224 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc
42808 ./obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib

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)

@euclio euclio force-pushed the euclio:nonstandard-style-suggestions branch from 4179fb7 to 81fd363 Jan 7, 2019

let left = snippet.find('"');
let right = snippet.rfind('"').map(|pos| snippet.len() - pos);

if let (Some(left), Some(right)) = (left, right) {

This comment has been minimized.

@oli-obk

oli-obk Jan 7, 2019

Contributor

you can just add a ? to the initializer of left and right instead of pattern matching.

This comment has been minimized.

@euclio

euclio Jan 7, 2019

Contributor

Nice, fixed.

Vec::new(),
Vec::new()
vec![
// #[allow(non_snake_case)]

This comment has been minimized.

@oli-obk

oli-obk Jan 7, 2019

Contributor

While I generally have nothing against this, I'm wondering if we should do an in_macro in the lint instead.

This comment has been minimized.

@euclio

euclio Jan 7, 2019

Contributor

I think the diagnostic is already not reported in macros? Is this just something funky with syntax extensions?

This comment has been minimized.

@oli-obk

oli-obk Jan 7, 2019

Contributor

Oh, right. So it's probably

let sym = Ident::with_empty_ctxt(Symbol::gensym(&format!(
using a dummy span instead of the span from

This comment has been minimized.

@euclio

euclio Jan 7, 2019

Contributor

with_empty_ctxt already creates an identifier with a dummy span, so won't the lint get triggered anyways? Seems eaiser to me to just allow the lint 😅 I think that's what happened with lint failures inside format!: they were just fixed.

This comment has been minimized.

@oli-obk

oli-obk Jan 8, 2019

Contributor

So... if it's a dummy, why is the lint triggered at all? Did you maybe do this change before you added the dummy check and the allow is now not necessary anymore?

This comment has been minimized.

@euclio

euclio Jan 8, 2019

Contributor

Ok, I was able to fix this by ensuring the syntax extensions use the call-site span with a macro expansion mark instead of just call-site span.

To clarify what I said earlier, the dummy check that I added only ensures that the suggestion gets added, not that the lint fires. span_helps don't get emitted with a dummy span, but the lint will. This issue had nothing to do with the dummy span check.

This comment has been minimized.

@oli-obk

oli-obk Jan 9, 2019

Contributor

Thanks! That also explains why the lint suddenly started getting reported.

@euclio euclio force-pushed the euclio:nonstandard-style-suggestions branch from 81fd363 to 9e436f9 Jan 7, 2019

euclio added some commits Jan 4, 2019

improve non_snake_case diagnostics
Use a structured suggestion and tighten the span to just the identifier.
improve non_upper_case_globals diagnostics
Use a structured suggestion and tighten the span to just the identifier.
improve non_camel_case_types diagnostics
Use a structured suggestion and tighten the span to just the identifier.

@euclio euclio force-pushed the euclio:nonstandard-style-suggestions branch from 9e436f9 to 1b28f5a Jan 8, 2019

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 9, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 9, 2019

📌 Commit 1b28f5a has been approved by oli-obk

@davidtwco davidtwco assigned oli-obk and unassigned davidtwco Jan 9, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Testing commit 1b28f5a with merge 59f835d...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57387 - euclio:nonstandard-style-suggestions, r=oli-obk
Use structured suggestions for nonstandard style lints

This PR modifies the lints in the nonstandard_style group to use structured suggestions. Note that there's a bit of tricky span calculation going on for the `crate_name` attribute. It also simplifies the code a bit: I don't think the "fallback" suggestions for these lints can actually be triggered.

Fixes #48103.
Fixes #52414.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 10, 2019

@bors retry
AppVeyor... what's wrong with you today?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

⌛️ Testing commit 1b28f5a with merge 1d029c6...

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57387 - euclio:nonstandard-style-suggestions, r=oli-obk
Use structured suggestions for nonstandard style lints

This PR modifies the lints in the nonstandard_style group to use structured suggestions. Note that there's a bit of tricky span calculation going on for the `crate_name` attribute. It also simplifies the code a bit: I don't think the "fallback" suggestions for these lints can actually be triggered.

Fixes #48103.
Fixes #52414.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 1d029c6 to master...

@bors bors merged commit 1b28f5a into rust-lang:master Jan 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@euclio euclio deleted the euclio:nonstandard-style-suggestions branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment