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

AST validation doesn't correctly deal with impls nested within associated functions #119924

Open
fmease opened this issue Jan 13, 2024 · 3 comments
Assignees
Labels
A-associated-items Area: Associated items (types, constants & functions) C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fmease
Copy link
Member

fmease commented Jan 13, 2024

The following programs get wrongfully rejected since my PR #119505 (nightly-2024-01-04):

pub struct S;

trait Trait {
    fn provided() {
        impl S {
            pub fn perform() {} //~ ERROR visibility qualifiers are not permitted here
        }
    }
}
pub struct S;
struct Expr<const N: u32>;

trait Trait {
    fn required(_: Expr<{
        impl S {
            pub fn perform() {} //~ ERROR visibility qualifiers are not permitted here
        }
        0
    }>);
}

The following program was incorrectly rejected even before my PR #119505 (nightly-2024-01-04):

#![feature(const_trait_impl, effects)]

pub struct S;

#[const_trait]
trait Trait {
    fn required();
}

impl const Trait for () {
    fn required() {
        impl S {
            pub fn perform() {} //~ ERROR visibility qualifiers are not permitted here
        }
    }
}

The following programs lead to an ICE even before my PR (e.g., in nightly-2023-12-31):

#![feature(const_trait_impl, effects)]

pub struct S;
#[const_trait]
trait Trait {
    fn provided() {
        impl S {
            fn perform<T: ~const Trait>() {} // should've gotten rejected during AST validation
            //~^ ICE no host param id for call in const yet no errors reported
        }
    }
}
#![feature(const_trait_impl, effects)]

pub struct S;
struct Expr<const N: u32>;

#[const_trait]
trait Trait {
    fn required(_: Expr<{
        impl S {
            fn perform<T: ~const Trait>() {} // should've gotten rejected during AST validation
            //~^ ICE no host param id for call in const yet no errors reported
        }
	0
    }>);
}
#![feature(const_trait_impl, effects)]

struct S;
#[const_trait]
trait Trait<const N: u32> {}

const fn f<T: Trait<{
    struct I<U: ~const Trait<0>>(U); // should've gotten rejected during AST validation
    //~^ ICE no host param id for call in const yet no errors reported
    0
}>>() {}

There are many more issues and probably many more ways to reproduce this, e.g. we don't visit attributes on associated functions under certain circumstances (I couldn't find a reproducer yet in which you can observe the bug).

@fmease fmease added A-associated-items Area: Associated items (types, constants & functions) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jan 13, 2024
@fmease fmease self-assigned this Jan 13, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 13, 2024
@fmease fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 13, 2024
bkchr added a commit to paritytech/polkadot-sdk that referenced this issue Jan 18, 2024
Apparently they changed detection for visibility identifiers on traits,
which broke more than it should. There is an issue open: rust-lang/rust#119924
The easy solution for us is to move the declaration of the global
variable outside of the trait.

Closes: #2960
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Jan 18, 2024
Apparently they changed detection for visibility identifiers on traits,
which broke more than it should. There is an issue open:
rust-lang/rust#119924 The easy solution for us
is to move the declaration of the global variable outside of the trait.

Closes: #2960
@fmease
Copy link
Member Author

fmease commented Jan 23, 2024

Ah nice, I found another one reported by someone else ages ago while triaging needs-triage-legacy issues: #89342.

@fmease
Copy link
Member Author

fmease commented Feb 2, 2024

Lol, no code path properly accounts for AnonConst in AstValidator:

fn f(_: impl Trait<{
    fn g(_: impl Sized) {} //~ ERROR nested `impl Trait` is not allowed
    false
}>) {}
trait Trait<const B: bool> {}

@fmease
Copy link
Member Author

fmease commented Feb 6, 2024

Fun, the following also gets rejected (not strictly AST validation but AST passes, namely feature_gate):

struct Const<const N: u32>;

type T = Const<{
    fn take(_: impl Sized) {}
    0
}>;
error[E0658]: `impl Trait` in type aliases is unstable
 --> src/lib.rs:4:16
  |
4 |     fn take(_: impl Sized) {}
  |                ^^^^^^^^^^
  |
  = note: see issue #63063 <https://github.com/rust-lang/rust/issues/63063> for more information
  = help: add `#![feature(type_alias_impl_trait)]` to the crate attributes to enable
  = note: this compiler was built on 2024-02-01; consider upgrading it if it is out of date

pmikolajczyk41 pushed a commit to Cardinal-Cryptography/polkadot-sdk that referenced this issue Feb 21, 2024
Apparently they changed detection for visibility identifiers on traits,
which broke more than it should. There is an issue open:
rust-lang/rust#119924 The easy solution for us
is to move the declaration of the global variable outside of the trait.

Closes: paritytech#2960
(cherry picked from commit 0e124a0)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 7, 2024
…piler-errors

AST validation: Improve handling of inherent impls nested within functions and anon consts

Minimal fix for issue rust-lang#121607 extracted from PR rust-lang#120698 for ease of backporting and since I'd like to improve PR rust-lang#120698 in such a way that it makes AST validator truly robust against such sort of regressions (AST validator is generally *beyond* footgun-y atm). The current version of PR rust-lang#120698 sort of does that already but there's still room for improvement.

Fixes rust-lang#89342.
Fixes [after beta-backport] rust-lang#121607.
Partially addresses rust-lang#119924 (rust-lang#120698 aims to fully fix it).

---

### Explainer

The last commit of PR rust-lang#119505 regressed issue rust-lang#121607.

Previously we would reject visibilities on associated items with `visibility_not_permitted` if we were in a trait (by checking the parameter `ctxt` of `visit_assoc_item` which was 100% accurate) or if we were in a trait impl (by checking a flag called `in_trait_impl` tracked in `AstValidator` which was/is only accurate if the visitor methods correctly updated it which isn't actually the case giving rise to the old open issue rust-lang#89342).

In PR rust-lang#119505, I moved even more state into the `AstValidator` by generalizing the flag `in_trait_impl` to `trait_or_trait_impl` to be able to report more precise diagnostics (modeling *Trait | TraitImpl*). However since we/I didn't update `trait_or_trait_impl` in all places to reflect reality (similar to us not updating `in_trait_impl` before), this lead to rust-lang#121607 (comment) getting wrongfully rejected. Since PR rust-lang#119505 we reject visibilities if the “globally tracked” (wrt. to `AstValidator`) `outer_trait_or_trait_impl` is `Some`.

Crucially, when visiting an inherent impl, I never reset `outer_trait_or_trait_impl` back to `None` leading us to believe that `bar` in the stack [`trait Foo` > `fn foo` > `impl Bar` > `pub fn bar`] (from the MCVE) was an inherent associated item (we saw `trait Foo` but not `impl Bar` before it).

The old open issue rust-lang#89342 is caused by the aforementioned issue of us never updating `in_trait_impl` prior to my PR rust-lang#119505 / `outer_trait_or_trait` after my PR. Stack: [`impl Default for Foo` > `{` > `impl Foo` > `pub const X`] (we only saw `impl Default for Foo` but not the `impl Foo` before it).

---

This PR is only meant to be a *hot fix*. I plan on completely *rewriting* `AstValidator` from the ground up to not rely on “globally tracked” state like this or at least make it close to impossible to forget updating it when descending into nested items (etc.). Other visitors do a way better job at that (e.g. AST lowering). I actually plan on experimenting with moving more and more logic from `AstValidator` into the AST lowering pass/stage/visitor to follow the [Parse, don't validate](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) “pattern”.

---

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 8, 2024
Rollup merge of rust-lang#122004 - fmease:astvalidator-min-fix, r=compiler-errors

AST validation: Improve handling of inherent impls nested within functions and anon consts

Minimal fix for issue rust-lang#121607 extracted from PR rust-lang#120698 for ease of backporting and since I'd like to improve PR rust-lang#120698 in such a way that it makes AST validator truly robust against such sort of regressions (AST validator is generally *beyond* footgun-y atm). The current version of PR rust-lang#120698 sort of does that already but there's still room for improvement.

Fixes rust-lang#89342.
Fixes [after beta-backport] rust-lang#121607.
Partially addresses rust-lang#119924 (rust-lang#120698 aims to fully fix it).

---

### Explainer

The last commit of PR rust-lang#119505 regressed issue rust-lang#121607.

Previously we would reject visibilities on associated items with `visibility_not_permitted` if we were in a trait (by checking the parameter `ctxt` of `visit_assoc_item` which was 100% accurate) or if we were in a trait impl (by checking a flag called `in_trait_impl` tracked in `AstValidator` which was/is only accurate if the visitor methods correctly updated it which isn't actually the case giving rise to the old open issue rust-lang#89342).

In PR rust-lang#119505, I moved even more state into the `AstValidator` by generalizing the flag `in_trait_impl` to `trait_or_trait_impl` to be able to report more precise diagnostics (modeling *Trait | TraitImpl*). However since we/I didn't update `trait_or_trait_impl` in all places to reflect reality (similar to us not updating `in_trait_impl` before), this lead to rust-lang#121607 (comment) getting wrongfully rejected. Since PR rust-lang#119505 we reject visibilities if the “globally tracked” (wrt. to `AstValidator`) `outer_trait_or_trait_impl` is `Some`.

Crucially, when visiting an inherent impl, I never reset `outer_trait_or_trait_impl` back to `None` leading us to believe that `bar` in the stack [`trait Foo` > `fn foo` > `impl Bar` > `pub fn bar`] (from the MCVE) was an inherent associated item (we saw `trait Foo` but not `impl Bar` before it).

The old open issue rust-lang#89342 is caused by the aforementioned issue of us never updating `in_trait_impl` prior to my PR rust-lang#119505 / `outer_trait_or_trait` after my PR. Stack: [`impl Default for Foo` > `{` > `impl Foo` > `pub const X`] (we only saw `impl Default for Foo` but not the `impl Foo` before it).

---

This PR is only meant to be a *hot fix*. I plan on completely *rewriting* `AstValidator` from the ground up to not rely on “globally tracked” state like this or at least make it close to impossible to forget updating it when descending into nested items (etc.). Other visitors do a way better job at that (e.g. AST lowering). I actually plan on experimenting with moving more and more logic from `AstValidator` into the AST lowering pass/stage/visitor to follow the [Parse, don't validate](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) “pattern”.

---

r? `@compiler-errors`
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
Apparently they changed detection for visibility identifiers on traits,
which broke more than it should. There is an issue open:
rust-lang/rust#119924 The easy solution for us
is to move the declaration of the global variable outside of the trait.

Closes: paritytech#2960
@matthiaskrgr matthiaskrgr added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Apr 5, 2024
@matthiaskrgr matthiaskrgr added the S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items (types, constants & functions) C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants