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

allow(deprecated) is too coarse-grained, should take a path #62398

Open
RalfJung opened this issue Jul 5, 2019 · 11 comments
Open

allow(deprecated) is too coarse-grained, should take a path #62398

RalfJung opened this issue Jul 5, 2019 · 11 comments
Labels
A-attributes Area: #[attributes(..)] A-frontend Area: frontend (errors, parsing and HIR) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

007d87f added allow(deprecated) in a few places because mem::uninitialized is still used. Unfortunately, this allows all deprecated items, so by the time someone gets around to fix that, other uses of deprecated items could have crept in.

It would be much better if that could be allow(deprecated(mem::uninitialized)) or so, to only allow the one method without also allowing everything else.

The same applies to deprecated_in_future.

I think I saw @Mark-Simulacrum ask for this somewhere recently? Or was it someone else?

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. A-frontend Area: frontend (errors, parsing and HIR) A-attributes Area: #[attributes(..)] A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. labels Jul 5, 2019
@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

Unfortunately, this allows all deprecated items, so by the time someone gets around to fix that, other uses of deprecated items could have crept in.

In most of these cases the allow(deprecated) lint check directive was applied on a specific statement or it was applied inside small tests. Only in the case of sgx and cloudabi was #![allow(deprecated)] applied inside a larger whole module. This is due to the highly specialized nature of the libstd codebase in terms of platform support.

I would like to see stronger justification for changing the grammar of allow(...) and friends, what other circumstances other than deprecated this would apply to, and whether this is a sufficiently large problem outside this repository.

@petrochenkov
Copy link
Contributor

ask for this somewhere recently? Or was it someone else?

Me? #61879 (comment)

In most of these cases the allow(deprecated) lint check directive was applied on a specific statement

Deprecation lints reported early (e.g. in #62042) don't currently have have the necessary infra to suppress them locally and have to be allowed at the crate level.

@petrochenkov
Copy link
Contributor

and whether this is a sufficiently large problem outside this repository.

I don't think this problem is specific to rustc.
Some uses of deprecated features are hard to get rid of, or maintainer may lack resources for any non-trivial work, and it may take weeks or months to address and they have to be allowed in the meantime.
At the same time, you want to be aware of other new deprecations, that may be trivial to fix.

deprecated(mem::uninitialized)

I'd prefer a feature name, or something like that.
With paths you immediately get the problem of resolving them.

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

Deprecation lints reported early (e.g. in #62042) don't currently have have the necessary infra to suppress them locally and have to be allowed at the crate level.

But in these specific cases these deprecation lints are not reported early, right? (Otherwise they would have had no effect in the #[allow(deprecated)] cases in the referenced commit.)

While from a complexity perspective (compiler + user facing) an extension to allow(...) is not too large, on first inspection it seems to me that what seems like a compiler bug should be fixed before considering language changes.

@petrochenkov
Copy link
Contributor

But in these specific cases these deprecation lints are not reported early, right? (Otherwise they would have had no effect in the #[allow(deprecated)] cases in the referenced commit.)

Hm, the only allows I see in that PR are at the crate level (namely, in doc tests - each doc test is built as a separate crate).

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

Hm, the only allows I see in that PR are at the crate level (namely, in doc tests - each doc test is built as a separate crate).

That's strange, the commit aforementioned has e.g.:

                #[allow(deprecated)]
                let mut loses_info: llvm::Bool = ::std::mem::uninitialized();

Some uses of deprecated features are hard to get rid of, or maintainer may lack resources for any non-trivial work, and it may take weeks or months to address and they have to be allowed in the meantime.
At the same time, you want to be aware of other new deprecations, that may be trivial to fix.

No doubt, but using #[allow(deprecated)] on the function or statement level seems like a decently an adequate way of achieving this even if it requires some copy pasting. It's not as fast as #![allow(deprecated(path::to::thing))] but it at least encourages you to fix some of the easy ones sooner.

I'd prefer a feature name, or something like that.
With paths you immediately get the problem of resolving them.

I'm against any mention of feature names in the stable language for the same reason I was opposed to #[cfg(rust_feature = "async_await)] as compared to #[cfg(accessible(::path::to::thing))]. The feature names are neither stable nor intuitive and are hastily made up throw-away symbols invented when RFCs or PRs are written.

@petrochenkov
Copy link
Contributor

That's strange, the commit aforementioned has e.g.:

Ah, we are talking about different PRs, I was talking about #62042.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 5, 2019

@petrochenkov ah right, it was you. :) Sorry.

In most of these cases the allow(deprecated) lint check directive was applied on a specific statement or it was applied inside small tests.

Another instance of (the equivalent of) a crate-level allow:

//#![warn(deprecated_in_future)] // FIXME: std still has quite a few uses of `mem::uninitialized`

If we had finer-grained control, that could have been

#![warn(deprecated_in_future)]
#![allow(deprecated_in_future(mem::uninitialized)]

Generally, when updating rustc (even for your own crate) I feel it can be hard to find all the right places for where to locally allow(deprecated) in a well-targeted way that minimizes accidentally allowing more. Just being able to allow all deprecated methods, to me, feels just as if we only were able to allow all warnings. There's a reason we have allow(some_lint) even though people could just very locally add #[allow] in the statements where they do something that's linted against. The same reason applies to allow(deprecated(path)).

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

@RalfJung In the interim, deprecated_in_future is rustc specific so we can allow this in an unstable fashion just for rustc internals without an RFC.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 5, 2019

Not sure what you mean? That lint can also be used by user crates.

But user crates can indeed not mark items as "deprecated in the future". Still, I don't know what it is you are proposing.

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

Still, I don't know what it is you are proposing.

I'm not really actively proposing it, but if you want to... we can add a rustc internal unstable feature gate under which you can do allow(deprecated_in_future(mem::uninitialized)) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] A-frontend Area: frontend (errors, parsing and HIR) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants