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

normalize substs while inlining #77306

Merged
merged 4 commits into from
Oct 18, 2020
Merged

normalize substs while inlining #77306

merged 4 commits into from
Oct 18, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 28, 2020

fixes #68347 or more precisely, this fixes the same ICE in rust analyser as veloren is pinned to a specific nightly
and had an error with the current one.

I didn't look into creating an MVCE here as that seems fairly annoying, will spend a few minutes doing so rn. (failed)

r? @eddyb cc @bjorn3

@eddyb
Copy link
Member

eddyb commented Sep 28, 2020

Hmm, this might be expensive. What I would do is assert that normalizing doesn't change the type, then track down the source of the unnormalized type and fix that. But just to see what impact it has on its own:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 28, 2020

⌛ Trying commit 9221cff8dd27d91159a85a5be10de376d4dd6df1 with merge 9f6cf14e9aa588419e60b7f07767d8d81befe061...

@bors
Copy link
Contributor

bors commented Sep 28, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9f6cf14e9aa588419e60b7f07767d8d81befe061 (9f6cf14e9aa588419e60b7f07767d8d81befe061)

@rust-timer
Copy link
Collaborator

Queued 9f6cf14e9aa588419e60b7f07767d8d81befe061 with parent fc2daaa, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9f6cf14e9aa588419e60b7f07767d8d81befe061): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@eddyb
Copy link
Member

eddyb commented Sep 29, 2020

LGTM but r? @nikomatsakis or @matthewjasper (this feels like a matter of policy, i.e. fixing normalization bugs or working around them with aggressive normalization elsewhere, like in this PR).

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Sep 29, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Sep 29, 2020

will spend a bit of time looking at where we fail to normalize this week to fix his bug at its origin

@nikomatsakis
Copy link
Contributor

The change itself seems harmless. I am of two minds about whether this sort of workaround is acceptable. In general I prefer to normalize at the source but I also feel like being pragmatic may make sense. It's too bad there is no test here but I see that @lcnr already tried to create one and failed.

@nikomatsakis
Copy link
Contributor

I guess I would say that if some effort at investigation fails, I am inclined to r+

@lcnr
Copy link
Contributor Author

lcnr commented Oct 4, 2020

Minimization

pub fn write() {
    create()()
}

pub fn create() -> impl FnOnce() {
   || ()
}

edit: this minimization doesn't actually cause an ICE on nightly but does cause one with the debug assert in codegen_fulfill_obligation.

@lcnr lcnr changed the title normalize in codegen_fulfill_obligations normalize substs while inlining Oct 4, 2020
@eddyb
Copy link
Member

eddyb commented Oct 4, 2020

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Oct 4, 2020

📌 Commit 73424ec396b743a8ca6ae74702ba1b9bcdce6919 has been approved by eddyb

@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 Oct 4, 2020
@lqd
Copy link
Member

lqd commented Oct 4, 2020

Here's a reduction of the veloren error:

pub fn main() {
    let _x: fn() = handle_debug_column;
}

fn handle_debug_column() {
    let sampler = sample_columns();

    let foo = || {
        sampler.get(17);
    };
    foo();
}

fn sample_columns() -> impl Sampler {
    ColumnGen {}
}

struct ColumnGen {}

trait Sampler {
    fn get(&self, index: i32);
}

impl Sampler for ColumnGen {
    fn get(&self, _index: i32) {}
}

@lcnr
Copy link
Contributor Author

lcnr commented Oct 4, 2020

added the test by @lqd

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Oct 4, 2020

📌 Commit 1f74e0b24f4bb64ad52b5addcc53654c56a8323f has been approved by eddyb

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Testing commit 1f74e0b24f4bb64ad52b5addcc53654c56a8323f with merge 96640e765bbab1ba6d7e0095b6d68a4af10a06fc...

@bors
Copy link
Contributor

bors commented Oct 5, 2020

💔 Test failed - checks-actions

@lcnr lcnr removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 7, 2020
@eddyb
Copy link
Member

eddyb commented Oct 18, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 18, 2020

📌 Commit ac893b8 has been approved by eddyb

@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 Oct 18, 2020
@bors
Copy link
Contributor

bors commented Oct 18, 2020

⌛ Testing commit ac893b8 with merge 4d247ad...

@bors
Copy link
Contributor

bors commented Oct 18, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 4d247ad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 18, 2020
@bors bors merged commit 4d247ad into rust-lang:master Oct 18, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 18, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #77306!

Tested on commit 4d247ad.
Direct link to PR: #77306

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).
💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro @calebcartwright).
💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro @calebcartwright).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Oct 18, 2020
Tested on commit rust-lang/rust@4d247ad.
Direct link to PR: <rust-lang/rust#77306>

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).
💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro @calebcartwright).
💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro @calebcartwright).
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 20, 2020
Add some MIR-related regression tests

Closes rust-lang#68841
Closes rust-lang#75053
Closes rust-lang#76375
Closes rust-lang#77911

I think they're fixed by rust-lang#77306.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 20, 2020
Add some MIR-related regression tests

Closes rust-lang#68841
Closes rust-lang#75053
Closes rust-lang#76375
Closes rust-lang#77911

I think they're fixed by rust-lang#77306.
@lcnr lcnr deleted the inline-ok branch May 15, 2023 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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