Skip to content

Re-add raw_pointer_derive lint as a no-op #30346

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.

Sign up for GitHub

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

Jump to bottom
Closed
brson opened this issue Dec 11, 2015 · 16 comments
Closed

Re-add raw_pointer_derive lint as a no-op #30346

brson opened this issue Dec 11, 2015 · 16 comments
Assignees
@brson
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@brson
Copy link
Contributor

brson commented Dec 11, 2015

Removed in #29882, causing breakage to maidsafe_utilities.

I recall at some point we decided never to remove lints because of this very problem and simply convert them to no-ops.
@brson brson added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 11, 2015
@brson
Copy link
Contributor Author

brson commented Dec 11, 2015

Policy-wise this is similar but (imo) different to our policy of allowing changes to lints to break code. E.g. cap-lints won't affect removed lints.

@alexcrichton
Copy link
Member

It looks like the breakage is due to unknown_lints which is itself a lint, so this seems like it's covered by our lint changes policy. Along those lines I can't actually think of a technical reason for why we'd want to not remove lints?

I do vaguely recall reaching the conclusion at some point, however, about not removing lints. I don't remember the reason, however, so I'd want to discuss this a bit before reverting.

@brson
Copy link
Contributor Author

brson commented Dec 23, 2015

Two more crates broke in the latest report.

@bluss
Copy link
Member

bluss commented Dec 24, 2015

Don't we have a cargo feature that ensures that warnings (lints) changes are of very low impact, i.e. don't really break things that depend on the crate, for example.

@sfackler
Copy link
Member

Yep, lints are capped at allowed for dependencies. Changing this one to be a no-op and maybe adding a default-warn noop-lints lint or whatever seems like a good idea though. It seems excessive to stop compilation instead of just warning when this kind of thing happens.

@brson
Copy link
Contributor Author

brson commented Jan 6, 2016

Another regression popped up in this report. accumulator-0.1.3

@brson
Copy link
Contributor Author

brson commented Jan 6, 2016

Also containerof-0.2.1, memory_map-0.0.3.

Edit: oh accumulator and memory_map were already known from a previous report. I think containerof is new.

@brson
Copy link
Contributor Author

brson commented Jan 6, 2016

After updating the regression sheet I count 7 crates affected by this.

@brson brson self-assigned this Jan 6, 2016
@brson
Copy link
Contributor Author

brson commented Jan 6, 2016

@nikomatsakis and I discussed that we'd prefer removed lints indicate they were removed with a warning instead of saying 'unknown lint'. I may try to get a patch together that does this.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 7, 2016

triage: P-high

(it is questionable about whether this should be classified as a "regression" per se, in terms of what the impact actually is of this sort of change if you aren't opting into stronger lint settings. but anyway, we want to handle this case better.)

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Jan 7, 2016
@nikomatsakis
Copy link
Contributor

It seems there is a mechanism for registering that a lint has been removed, why didn't we use that? I think we ought to.

@brson
Copy link
Contributor Author

brson commented Jan 7, 2016

I will try adding it back as a removed lint and making sure it doesn't cause build failures.

@brson
Copy link
Contributor Author

brson commented Jan 11, 2016

I've gotten in to yak shaving with the code for registering renamed and removed lints. It doesn't have test coverage and has some issues. Still expect to get a patch in.

@brson brson mentioned this issue Jan 13, 2016
Merged
brson added a commit to brson/rust that referenced this issue Jan 14, 2016
@brson
rustc: Fix bugs in renamed and removed lints and re-add raw_pointer_d…
ca81d3d 
…erive

cc rust-lang#30346
bors added a commit that referenced this issue Jan 15, 2016
@bors
Auto merge of #30878 - brson:raw-pointer-derive, r=brson
cee9463 
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
brson added a commit to brson/rust that referenced this issue Jan 16, 2016
@brson
rustc: Fix bugs in renamed and removed lints and re-add raw_pointer_d…
a74d647 
…erive

cc rust-lang#30346

Conflicts:
	src/librustc_lint/lib.rs
@nikomatsakis
Copy link
Contributor

Is this not fixed?

@nikomatsakis
Copy link
Contributor

@brson ^^ :)

@alexcrichton
Copy link
Member

Done in #30878

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

@brson brson

Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@alexcrichton @brson @nikomatsakis @pnkfelix @sfackler @bluss @rust-highfive