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

save-analysis: Nest typeck tables when processing functions/methods #64250

Merged
merged 7 commits into from
Sep 16, 2019

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Sep 7, 2019

Fixes an issue where we did not nest tables correctly when resolving
associated types in formal argument/return type positions.

This was the minimized reproduction case that I tested the fix on:

pub trait Trait {
    type Assoc;
}

pub struct A;

pub fn func() {
    fn _inner1<U: Trait>(_: U::Assoc) {}
    fn _inner2<U: Trait>() -> U::Assoc { unimplemented!() }

    impl A {
        fn _inner1<U: Trait>(self, _: U::Assoc) {}
        fn _inner2<U: Trait>(self) -> U::Assoc { unimplemented!() }
    }
}

using debug_assertions-enabled rustc and by additionally passing -Zsave-analysis.

Unfortunately the original assertion fired is a debug one and from what I can tell we don't run the tests with these on, so I'm not adding a test here. If I missed it and there is a way to run tests with these on, I'd love to add a test case for this.

Closes #63663
Closes #50328
Closes #43982

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2019
@Mark-Simulacrum
Copy link
Member

We can probably change the assertion to be a non-debug assertion?

bors added a commit that referenced this pull request Sep 7, 2019
…es, r=<try>

[DONT MERGE] Always validate HIR ID for TypeckTables

...and not only with `debug_assertions` compiled in - #64250 (comment).

cc @Mark-Simulacrum @michaelwoerister

Let's do a try run to see if this impacts perf anyhow?

r? @ghost
@Xanewok
Copy link
Member Author

Xanewok commented Sep 7, 2019

We can probably change the assertion to be a non-debug assertion?

Started a try->perf run over at #64262

bors added a commit that referenced this pull request Sep 7, 2019
…es, r=<try>

[DONT MERGE] Always validate HIR ID for TypeckTables

...and not only with `debug_assertions` compiled in - #64250 (comment).

cc @Mark-Simulacrum @michaelwoerister

Let's do a try run to see if this impacts perf anyhow?

r? @ghost
Xanewok added a commit to Xanewok/rust that referenced this pull request Sep 8, 2019
Performance shouldn't be impacted (see [1] for a perf run) and this
should allow us to catch more bugs, e.g. [2] and [3].

[1]: rust-lang#64262
[2]: rust-lang#64250
[3]: rust-lang#57298
@Xanewok
Copy link
Member Author

Xanewok commented Sep 8, 2019

Perf run turned out to be okay, so I dropped the conditional compilation on debug assertions and added a test case.

@varkor
Copy link
Member

varkor commented Sep 10, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 10, 2019

📌 Commit 7730ef1 has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Sep 10, 2019
… r=varkor

save-analysis: Nest typeck tables when processing functions/methods

Fixes an issue where we did not nest tables correctly when resolving
associated types in formal argument/return type positions.

This was the minimized reproduction case that I tested the fix on:
```rust
pub trait Trait {
    type Assoc;
}

pub struct A;

pub fn func() {
    fn _inner1<U: Trait>(_: U::Assoc) {}
    fn _inner2<U: Trait>() -> U::Assoc { unimplemented!() }

    impl A {
        fn _inner1<U: Trait>(self, _: U::Assoc) {}
        fn _inner2<U: Trait>(self) -> U::Assoc { unimplemented!() }
    }
}
```
using `debug_assertions`-enabled rustc and by additionally passing `-Zsave-analysis`.

Unfortunately the original assertion fired is a *debug* one and from what I can tell we don't run the tests with these on, so I'm not adding a test here. If I missed it and there is a way to run tests with these on, I'd love to add a test case for this.

Closes rust-lang#63663
Closes rust-lang#50328
Closes rust-lang#43982
@Centril
Copy link
Contributor

Centril commented Sep 10, 2019

Failed in #64366 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 10, 2019
@Xanewok
Copy link
Member Author

Xanewok commented Sep 10, 2019

That's unfortunate, thanks for the heads up; will look into it tomorrow

Failure:

error: internal compiler error: src/librustc/ty/context.rs:211: node type <I>::Item (hir_id=HirId { owner: DefIndex(4366), local_id: 4 }) with HirId::owner DefId(0:4366 ~ core[db80]::iter[0]::adapters[0]::{{impl}}[25]::try_fold[0]::nth[0]::{{opaque}}[0]) cannot be placed in TypeckTables with local_id_root DefId(0:4364 ~ core[db80]::iter[0]::adapters[0]::{{impl}}[25]::try_fold[0]::nth[0])

Probably caused by

fn nth<I: Iterator>(iter: &mut I, step: usize) -> impl FnMut() -> Option<I::Item> + '_ {

EDIT: Minimized to:

pub trait Trait {
    type Assoc;
}

pub fn func() {
    fn _inner<U: Trait>() -> impl Fn(U::Assoc) { |_| unimplemented!() }
}

Fixes an issue where we did not nest tables correctly when resolving
associated types in formal argument/return type positions
Performance shouldn't be impacted (see [1] for a perf run) and this
should allow us to catch more bugs, e.g. [2] and [3].

[1]: rust-lang#64262
[2]: rust-lang#64250
[3]: rust-lang#57298
...since the code is literally the same and does the same thing.
@Xanewok
Copy link
Member Author

Xanewok commented Sep 14, 2019

This is updated and ready for review again.

The last bug was uncovered when processing type paths in return type impl Traits.

Specifically, this is because we try to resolve the qualified type path using the typeck tables but a debug assertion fires since the def id of the type path and that of the table don't share the same local id root.
That's because the impl Trait lowering creates a definition introducing a separate {{opaque}} def path segment with trait bounds as def children.

Unfortunately, the correct way under the current architecture would be to nest the tables further, however from what I can tell the newly lowered synthetic def path segment does not have typeck tables (probably rightly so? @nikomatsakis), so to prevent the underlying ICE and to enable the debug assertion for good now, I decided to only add explanatory comments and skip processing bounds in impl Traits in return type position for now.

As a side note, the current save-analysis infrastructure is slowly becoming dated as we introduce more supporting infrastructure with time (e.g. cleaning up resolution, definition infra from incremental), so it might be worth thinking how to handle it in the medium-term (leverage HIR directly instead of AST? maybe use it as a way to somehow bridge/push for librarification of the frontend?).

@jonas-schievink jonas-schievink added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2019
@jonas-schievink
Copy link
Contributor

This also fixes #57298 and #55172

@varkor
Copy link
Member

varkor commented Sep 15, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 15, 2019

📌 Commit a946b8d has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 15, 2019
… r=varkor

save-analysis: Nest typeck tables when processing functions/methods

Fixes an issue where we did not nest tables correctly when resolving
associated types in formal argument/return type positions.

This was the minimized reproduction case that I tested the fix on:
```rust
pub trait Trait {
    type Assoc;
}

pub struct A;

pub fn func() {
    fn _inner1<U: Trait>(_: U::Assoc) {}
    fn _inner2<U: Trait>() -> U::Assoc { unimplemented!() }

    impl A {
        fn _inner1<U: Trait>(self, _: U::Assoc) {}
        fn _inner2<U: Trait>(self) -> U::Assoc { unimplemented!() }
    }
}
```
using `debug_assertions`-enabled rustc and by additionally passing `-Zsave-analysis`.

Unfortunately the original assertion fired is a *debug* one and from what I can tell we don't run the tests with these on, so I'm not adding a test here. If I missed it and there is a way to run tests with these on, I'd love to add a test case for this.

Closes rust-lang#63663
Closes rust-lang#50328
Closes rust-lang#43982
bors added a commit that referenced this pull request Sep 15, 2019
Rollup of 3 pull requests

Successful merges:

 - #63872 (Document platform-specific behavior of the iterator returned by std::fs::read_dir)
 - #64250 (save-analysis: Nest typeck tables when processing functions/methods)
 - #64472 (Don't mark expression with attributes as not needing parentheses)

Failed merges:

r? @ghost
@bors bors merged commit a946b8d into rust-lang:master Sep 16, 2019
@flip1995
Copy link
Member

b456c82 lets Clippy ICE, when calling qpath_res on the HirId of a struct field type.

Call stack:

LateLintPass::check_struct_field(.., cx, field);
cx.tables.qpath_res(field.ty.node -> qpath, field.ty.hir_id);

This seems weird to me, since we're checking the QPath of a HIR node with the HirId of the same node. @Xanewok is this related to #64250 (comment)?

@Xanewok
Copy link
Member Author

Xanewok commented Sep 17, 2019

Left some explanation on why the assertion fires and how to fix it at rust-lang/rust-clippy#4545 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
9 participants