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

Do not hard error on broken constants in type aliases #82700

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 2, 2021

In #69741 @estebank was adding WF checks to type aliases. This will evaluate all constants in a type alias, which wasn't happening before, so that would be a breaking change. Thus I proactively excluded these constants from the hard error list, and we instead emit the future incompat lint. More context: #69741 (comment)

r? @lcnr

cc @rust-lang/wg-const-eval

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2021
@@ -311,7 +311,19 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
// <https://github.com/rust-lang/rust/issues/71800>.
let emit_as_lint = if let Some(def) = def.as_local() {
// (Associated) consts only emit a lint, since they might be unused.
matches!(tcx.def_kind(def.did.to_def_id()), DefKind::Const | DefKind::AssocConst)
matches!(tcx.def_kind(def.did.to_def_id()), DefKind::Const | DefKind::AssocConst) || {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaict this also only emits this as a lint even if we are using the associated const which doesn't seem desirable to me

what's the result for the following with this change?

type Foo = [u8; 0 - 1];

fn foo() -> Foo {
   todo!()
}

and

struct Q<const N: usize>;
type Foo = Q<{0 - 1}>;

impl Default for Foo {
    fn default() -> Self {
        Q
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these will become future incompat lints, too

@RalfJung
Copy link
Member

RalfJung commented Mar 2, 2021

Oh no, why does this error reporting logic just keep becoming more bespoke. :(

What is the long-term plan for this? const_err is supposed to become a hard error eventually, after all.

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the lint_bad_const_in_ty_alias branch from c4daa53 to 63aee75 Compare March 2, 2021 14:05
@crlf0710 crlf0710 added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-const-eval Area: constant evaluation (mir interpretation) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2021
@oli-obk oli-obk closed this Mar 29, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 29, 2021

Interpreting the thumbs up and comment on https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/erroneous.20constants.20in.20type.20alias/near/229492342 as we don't need this and can just hard error

@oli-obk oli-obk deleted the lint_bad_const_in_ty_alias branch March 29, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants