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

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

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

r? @varkor

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

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

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

bors added a commit that referenced this pull request Sep 7, 2019
Auto merge of #64262 - Xanewok:always-validate-hir-id-for-typeck-tabl…
…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

This comment has been minimized.

Copy link
Member Author

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
Auto merge of #64262 - Xanewok:always-validate-hir-id-for-typeck-tabl…
…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
Always validate HIR ID for TypeckTables
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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

📌 Commit 7730ef1 has been approved by varkor

tmandry added a commit to tmandry/rust that referenced this pull request Sep 10, 2019
Rollup merge of rust-lang#64250 - Xanewok:save-analysis-assoc-nested,…
… 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 10, 2019
Auto merge of #64366 - tmandry:rollup-vy8wdo1, r=tmandry
Rollup of 6 pull requests

Successful merges:

 - #64060 (Improve hygiene of `alloc::format!`)
 - #64072 (Replace file_stem by file_name in rustdoc markdown)
 - #64085 (Tweak unsatisfied HRTB errors)
 - #64188 (rustc: Allow the cdylib crate type with wasm32-wasi)
 - #64250 (save-analysis: Nest typeck tables when processing functions/methods)
 - #64349 (documentation for AtomicPtr CAS operations)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

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

@Xanewok

This comment has been minimized.

Copy link
Member Author

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!() }
}
Xanewok added 5 commits Sep 5, 2019
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
Always validate HIR ID for TypeckTables
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]: #64262
[2]: #64250
[3]: #57298
save-analysis: Use process_bounds when processing opaque impl item type
...since the code is literally the same and does the same thing.

@Xanewok Xanewok force-pushed the Xanewok:save-analysis-assoc-nested branch from 7730ef1 to 0fafd61 Sep 13, 2019

@Xanewok

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

This also fixes #57298 and #55172

@varkor

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

📌 Commit a946b8d has been approved by varkor

Centril added a commit to Centril/rust that referenced this pull request Sep 15, 2019
Rollup merge of rust-lang#64250 - Xanewok:save-analysis-assoc-nested,…
… 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
Auto merge of #64491 - Centril:rollup-21wkl69, r=Centril
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

4 checks passed

pr Build #20190914.45 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@flip1995

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

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 Xanewok deleted the Xanewok:save-analysis-assoc-nested branch Sep 17, 2019

@Xanewok

This comment has been minimized.

Copy link
Member Author

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
Projects
None yet
9 participants
You can’t perform that action at this time.