Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uprustc: Fix bugs in renamed and removed lints and re-add raw_pointer_derive #30878
Conversation
rust-highfive
assigned
nikomatsakis
Jan 13, 2016
brson
force-pushed the
brson:raw-pointer-derive
branch
from
e86f816
to
a13b094
Jan 13, 2016
brson
added
the
beta-nominated
label
Jan 13, 2016
brson
force-pushed the
brson:raw-pointer-derive
branch
from
a13b094
to
d46117b
Jan 13, 2016
nagisa
reviewed
Jan 13, 2016
| Level::Forbid => "-F", | ||
| }, | ||
| lint_name); | ||
| sess.note_without_error(&msg); |
This comment has been minimized.
This comment has been minimized.
nagisa
Jan 13, 2016
Contributor
Use a structural error (you probably would want to return one from check_lint_name in some form?)
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jan 13, 2016
Contributor
Ah, yes, this is true. It'd be better to return the error and attach the note to it -- that would also mean that the caller is always the one that ultimately reports all warnings/errors, which seems good.
nagisa
reviewed
Jan 13, 2016
| // The raw_pointer_derived lint only warns about its own removal | ||
| // cc #30346 | ||
|
|
||
| #[deny(raw_pointer_derive)] //~ WARN raw_pointer_derive has been removed |
This comment has been minimized.
This comment has been minimized.
nagisa
Jan 13, 2016
Contributor
I wonder whether the diagnostic for #[deny(raw_pointer_deriving)] would only report about it being renamed, or also that it has been removed? I suspect its former and I think we might want the behaviour resembling the later.
This comment has been minimized.
This comment has been minimized.
brson
Jan 14, 2016
Author
Contributor
You are probably right, but I don't think it's worth the effort to change. What will happen is they will get the renamed warning, change the name, then get the removed warning. It is a minor hassle for a scenario that is never going to come up in practice with this particular lint, and may never happen again.
nagisa
reviewed
Jan 13, 2016
| /// passed on the command line. Since it emits non-fatal warnings and | ||
| /// there are *two* lint passes that inspect attributes, this is only | ||
| /// run from the late pass to avoid printing duplicate warnings. | ||
| fn check_lint_name(sess: &Session, |
This comment has been minimized.
This comment has been minimized.
nagisa
Jan 13, 2016
Contributor
To me it seems strange that we report some warnings in this function and have callers to handle reporting warnings/errors for another cases.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jan 13, 2016
Contributor
If so, I presume @brson structured it this way because some callers want warnings, and some want errors. Seems ok to me (though it wouldn't hurt to put the reasoning into a comment).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
brson
Jan 14, 2016
Author
Contributor
The logic is just awkward. It's called in two scenarios and some of the errors/warnings are shared, some different. Doing the warning reporting in the caller would introduce duplication.
nikomatsakis
reviewed
Jan 13, 2016
| Ok, | ||
| // Lint doesn't exist | ||
| NoLint, | ||
| // The lint is either ranamed or removed and a warning was printed |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
reviewed
Jan 13, 2016
| @@ -152,8 +152,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { | |||
| store.register_late_pass(sess, false, box lint::GatherNodeLevels); | |||
|
|
|||
| // Insert temporary renamings for a one-time deprecation | |||
| store.register_renamed("raw_pointer_deriving", "raw_pointer_derive"); | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Jan 13, 2016
Contributor
Maybe make this register_removed, since it was ultimately removed?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This looks good to me, though I think that restructuring to return a |
brson
force-pushed the
brson:raw-pointer-derive
branch
from
d46117b
to
a9efbeb
Jan 14, 2016
This comment has been minimized.
This comment has been minimized.
|
r? @nikomatsakis I've addressed all points. |
brson
added
the
beta-accepted
label
Jan 14, 2016
This comment has been minimized.
This comment has been minimized.
|
@rust-lang/compiler I tagged this beta-accepted, assuming you would be cool with it. @nikomatsakis and I at least have discussed it a lot. |
This comment has been minimized.
This comment has been minimized.
|
r=me modulo nits, and I think backporting makes sense. Looks pretty non-invasive. |
brson
force-pushed the
brson:raw-pointer-derive
branch
from
a9efbeb
to
ca81d3d
Jan 14, 2016
This comment has been minimized.
This comment has been minimized.
|
r=nikomatsakis |
This comment has been minimized.
This comment has been minimized.
|
@bors r+ p=1 |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jan 15, 2016
brson commentedJan 13, 2016
This adds back the raw_pointer_derive lint as a 'removed' lint, so that its removal does not cause errors (#30346) but warnings.
In the process I discovered regressions in the code for renamed and removed lints, which didn't appear to have any tests. The addition of a second lint pass (ast vs. hir) meant that attributes were being inspected twice, renamed and removed warnings printed twice. I restructured the code so these tests are only done once and added tests. Unfortunately it makes the patch more complicated for the needed beta backport.
r? @nikomatsakis