Skip to content

save_analysis: fix some ICEs - #73046

Merged
bors merged 2 commits into
rust-lang:masterfrom
marmeladema:save-analysis-fix-path
Jun 8, 2020
Merged

save_analysis: fix some ICEs#73046
bors merged 2 commits into
rust-lang:masterfrom
marmeladema:save-analysis-fix-path

Conversation

@marmeladema

@marmelademamarmeladema commented Jun 5, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#73020
Fixes#73022
Fixes#73041

@rust-highfive

rust-highfive commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

r? @eddyb

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020
@marmeladema

marmeladema commented Jun 5, 2020

Copy link
Copy Markdown
ContributorAuthor

r? @Xanewok

@rust-highfiverust-highfive assigned Xanewok and unassigned eddybJun 5, 2020
@Xanewok

Xanewok commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

Good catch, thanks!

@bors r+ rollup

@bors

bors commented Jun 5, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 84e4777 has been approved by Xanewok

@borsbors 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 Jun 5, 2020
@marmelademamarmeladema changed the title save_analysis: fix ice in get_expr_datasave_analysis: fix some ICEsJun 5, 2020
@marmeladema

marmeladema commented Jun 5, 2020

Copy link
Copy Markdown
ContributorAuthor

Sorry @Xanewok i've just added a fix for another issue.

@Xanewok

Xanewok commented Jun 5, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Do you think there are going to be more issues with TypeRelative paths being not matched on?

By the way you can try running the entire test suite with -Zsave-analysis passed to considerably increase the test set 😄

@marmeladema

marmeladema commented Jun 5, 2020

Copy link
Copy Markdown
ContributorAuthor

Potentially yes, but i've tried adding it everywhere and it leads to other bugs later on in librustc_middle so it will take a bit of time to understand how to do it properly.

In the mean time, those 2 commits should fix some ices/panics.

@marmeladema

marmeladema commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

By the way you can try running the entire test suite with -Zsave-analysis passed to considerably increase the test set smile

That's what I am doing right now to find more crashes 👍 I hope to come back with some more patches in the next few days

@Xanewok

Xanewok commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

That's good to hear! Let's try to land this, then:

@bors r+

@bors

bors commented Jun 6, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 4d6a307 has been approved by Xanewok

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 6, 2020
@Dylan-DPC
Rollup merge ofrust-lang#73046- marmeladema:save-analysis-fix-path,…
22f530c
… r=Xanewok
save_analysis: fix some ICEs
Fixesrust-lang#73020Fixesrust-lang#73022Fixesrust-lang#73041
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Jun 6, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 6, 2020
@Dylan-DPC
Rollup merge ofrust-lang#73046- marmeladema:save-analysis-fix-path,…
450a1b6
… r=Xanewok
save_analysis: fix some ICEs
Fixesrust-lang#73020Fixesrust-lang#73022Fixesrust-lang#73041
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Jun 6, 2020
Closed
tesuji
tesuji reviewed Jun 6, 2020
View reviewed changes
Comment threadsrc/librustc_save_analysis/dump_visitor.rs Outdated
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
@RalfJung
Rollup merge ofrust-lang#73046- marmeladema:save-analysis-fix-path,…
e9e7a61
… r=Xanewok
save_analysis: fix some ICEs
Fixesrust-lang#73020Fixesrust-lang#73022Fixesrust-lang#73041
@RalfJungRalfJung mentioned this pull request Jun 6, 2020
Closed
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
@RalfJung
Rollup merge ofrust-lang#73046- marmeladema:save-analysis-fix-path,…
473e1a9
… r=Xanewok
save_analysis: fix some ICEs
Fixesrust-lang#73020Fixesrust-lang#73022Fixesrust-lang#73041
@RalfJungRalfJung mentioned this pull request Jun 6, 2020
Closed
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
@RalfJung
Rollup merge ofrust-lang#73046- marmeladema:save-analysis-fix-path,…
9f4fce1
… r=Xanewok
save_analysis: fix some ICEs
Fixesrust-lang#73020Fixesrust-lang#73022Fixesrust-lang#73041
@RalfJung

RalfJung commented Jun 6, 2020

Copy link
Copy Markdown
Member

3 rollups failed with the same error and all contained this PR:
#73052, #73053, #73062
So, I think this is likely the culprit...
@bors r- rollup=never

@borsbors 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 Jun 6, 2020
@marmeladema

marmeladema commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Superseded by #73071

@RalfJung

RalfJung commented Jun 6, 2020

Copy link
Copy Markdown
Member

(Looks like that's just more commits added to this one, so IMO it would make sense to keep using the same PR so that all discussion is in one place. But whatever.^^)

@marmeladema

marmeladema commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

The hope is that those new commits will magically fix those weird failures :-D

@RalfJung

RalfJung commented Jun 6, 2020

Copy link
Copy Markdown
Member

I understand. Still no reason to open a new PR -- to the contrary, opening a new PR erases the prior discussion and thus the relation to the previous failures.

But also, another rollup saw the same failures again and didn't include this PR. I am at a total loss where the problem is coming from.

@marmeladema

marmeladema commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

I understand and I don't mind to let this PR open as a matter of fact. I thought it would be confusing to have two that includes the same commits.

@marmelademamarmeladema reopened this Jun 6, 2020
@RalfJung

RalfJung commented Jun 6, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Yes that's confusing indeed. So why did you open a 2nd one in the first place? You had a perfectly fine PR here, no reason to ditch it. :)

@RalfJungRalfJung mentioned this pull request Jun 6, 2020
Closed
@marmeladema

marmeladema commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

So @RalfJung let's go as originally planned. Can this be approved again? Or is there still suspicions this PR is breaking CI?

@Xanewok

Xanewok commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

r=me with https://github.com/rust-lang/rust/pull/73046/files#r436305949 addressed

@marmeladema
marmelademaforce-pushed the save-analysis-fix-path branch from 4d6a307 to a7c18e0CompareJune 6, 2020 22:40
@marmeladema
marmeladema requested a review from XanewokJune 6, 2020 22:42
@Xanewok

Xanewok commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

Thanks a lot! Let's try to get this in again...

@bors r+

@bors

bors commented Jun 6, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit a7c18e0 has been approved by Xanewok

@borsbors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 6, 2020
@marmelademamarmeladema mentioned this pull request Jun 7, 2020
Closed
@Dylan-DPC-zz

Dylan-DPC-zz commented Jun 8, 2020

Copy link
Copy Markdown

@bors p=1

@bors

bors commented Jun 8, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a7c18e0 with merge 7355816...

@bors

bors commented Jun 8, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-azure
Approved by: Xanewok
Pushing 7355816 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jun 8, 2020
@bors
bors merged commit 7355816 into rust-lang:masterJun 8, 2020
@marmeladema
marmeladema deleted the save-analysis-fix-path branch April 24, 2021 09:12
@cuvipercuviper added this to the 1.46 milestone May 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XanewokXanewokAwaiting requested review from Xanewok
1 more reviewer
@tesujitesujitesuji left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@XanewokXanewok

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.46

9 participants

@marmeladema@rust-highfive@Xanewok@bors@RalfJung@Dylan-DPC-zz@tesuji@cuviper@eddyb