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

provide suggestion for incorrect deprecated syntax #56896

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@euclio
Copy link
Contributor

euclio commented Dec 16, 2018

Fixes #48271.

r? @estebank

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Dec 17, 2018

I'd r+, but I'm worried about anyone out there incorrectly using deprecated, as it is usable by 3rd party crates since 1.9.0.

I'll do a crater run to make sure we don't blow anything up, otherwise happy with the change. If there's lots of usage in the wild, this will have to be changed to be a deprecation lint (like #55373).

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

⌛️ Trying commit d4aea02 with merge bad3651...

bors added a commit that referenced this pull request Dec 17, 2018

Auto merge of #56896 - euclio:deprecated-note-suggestion, r=<try>
provide suggestion for incorrect deprecated syntax

Fixes #48271.

r? @estebank
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 18, 2018

💥 Test timed out

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Dec 24, 2018

@craterbot run start=master#adbfec229ce07ff4b2a7bf2d6dec2d13cb224980 end=try#bad365140e1e8233b42b21af70a407f14ce5fec5 mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 24, 2018

👌 Experiment pr-56896 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 24, 2018

🚧 Experiment pr-56896 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 25, 2018

🎉 Experiment pr-56896 is completed!
📊 205 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Dec 26, 2018

@euclio It seems like we'll have to turn it into a warning or (ideally) lint, and reach out to arc-swap to fix their use of deprecated.

@vorner

This comment has been minimized.

Copy link
Contributor

vorner commented Dec 26, 2018

Sorry for the problems :-|. I've released a fix for that.

But going through the crater report randomly, it seems there are other ones too.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Dec 27, 2018

@vorner no need to apologize, if we didn't want you to write the attribute that way we should have made it a hard error :)

@euclio do you think you'll be able to turn this into a lint? That way we can merge it as allow by default, and a few releases down the line enable it as warn and finally deny by default.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 27, 2018

Hmm... Even tho it wasn't mandated by an RFC, given that #[deprecated = "foobar"] is already supported in the language, I think It'd rather have this fixed in rustdoc instead of removing #[deprecated = "foobar"] from the language. It seems harmless to support and it reduces churn in libraries.

cc @rust-lang/lang @rust-lang/libs

@Centril Centril added the T-lang label Dec 27, 2018

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 27, 2018

Not only is it harmless, I think this is the only reasonable option-- lots of code has used #[deprecated = "...."] today without knowing that it was the "incorrect" way of writing that code. Starting to error on that code would be a breaking change for essentially no reason (this isn't unsound, difficult to support, etc. and it doesn't interact poorly with other language features AFAIK), which seems like a violation of our backwards-compatibility guarantees.

(note: not all code is checked by crater, either-- this change would break several crates inside of Fuchsia that were previously using #[deprecated = ...], and received no warning)

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Dec 27, 2018

I would prefer going with a future-incompat lint here instead of deviating from the RFC. deprecated = "1.2.3" seems just as reasonable to me syntactically and that would be incorrectly interpreted as the note if we begin to accept that syntax. Furthermore, It's trivial to fix this with cargo fix during the warning period.

As an aside, attribute parsing is extremely lax in various places in the compiler. I think that we should require stricter parsing tests for stabilization of new attributes to avoid this situation in the future.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

@euclio

I would prefer going with a future-incompat lint here instead of deviating from the RFC.

That has to be measured against a) the churn to libraries it will cause, b) the negative reputation wrt. stability. It does not seem clear to me that making it an eventual error is right.

deprecated = "1.2.3" seems just as reasonable to me syntactically and that would be incorrectly interpreted as the note if we begin to accept that syntax.

I don't think deprecated = "1.2.3" is too problematic; the worst thing that could happen is that the user sees:

warning: use of deprecated item 'foo': 1.2.3

which while not obvious to everyone wrt. intent is still better than nothing.

We could also implement a heuristic in the compiler that understands the format \d+.\d+(.\d+)?.

Given @cramertj's concurrence I move that we:

@rfcbot close

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 28, 2018

Team member @Centril has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Dec 28, 2018

@Centril I would still like to add it as a lint, so that individual projects can require the newer syntax at will. I don't think we could change this to deny-by-default until the next epoch.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

@Centril I would still like to add it as a lint, so that individual projects can require the newer syntax at will.

I would be OK with an allow-by-default lint or to do it in clippy; The FCP is for not erroring.

I don't think we could change this to deny-by-default until the next epoch.

I don't think we should change it even in Rust 2021.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Dec 28, 2018

To be clear, I don't think we should error either. I'm arguing for a warn-by-default future-incompat lint.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

@euclio

To be clear, I don't think we should error either. I'm arguing for a warn-by-default future-incompat lint.

A warn-by-default C-future-compatibility lint would mean that we intend to eventually make it a hard error.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Dec 28, 2018

Yes, that would be my intention. Likely in the next edition.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

Right; what I'm saying is that I don't think we should make it an error ever because the churn doesn't seem worth it and doing it in an edition would make it even more complicated than supporting it. :)

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Dec 28, 2018

Either way I'd rather warn and fix it automatically via cargo fix than accept the incorrect syntax with an allow-by-default lint that is likely to be left undiscovered, since this is clearly a bug in the implementation.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

@euclio My preference is that #[deprecated = "abc" should just be synonymous with #[deprecated(note = "abc")] and if it isn't, then that's the direction in which we should fix things. In other words, it's no longer incorrect syntax, but an alternative, correct syntax.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Dec 28, 2018

I understand, but I'd prefer to stick with the syntax specified in the RFC. I'll defer to the Lang team's decision, however.

@vorner

This comment has been minimized.

Copy link
Contributor

vorner commented Dec 28, 2018

If I may add my humble 2 cents, there's something good on „one correct way to do things“ so if the #[deprecated = "note"] is somehow discouraged, I'd personally prefer that to be warn-by-default (and maybe leave it as that forever ‒ there are lints that never plan to become hard errors, like dead-code or deprecated items in the standard library).

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 9, 2019

I'd be happy with keeping the deprecated syntax for deprecated, and treating it as the note, as long as we have a warn-by-default lint for the very specific case where the value looks like a semver.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 9, 2019

@joshtriplett That seems fine to me tho I wonder if it will arise in practice?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

☔️ The latest upstream changes (presumably #57321) made this pull request unmergeable. Please resolve the merge conflicts.

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