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

Recover on const impl<> X for Y #79668

Merged
merged 1 commit into from
Dec 13, 2020
Merged

Conversation

coolreader18
Copy link
Contributor

@coolreader18 coolreader18 commented Dec 3, 2020

@leonardo-m mentioned that const impl Foo for Bar could be recovered from in #79287.

I'm not sure about the error strings as they are, I think it should probably be something like the error that expected_one_of_not_found makes + the suggestion to flip the keywords, but I'm not sure how exactly to do that. Also, I decided not to try to handle const unsafe impl or unsafe const impl cause I figured that unsafe impl const would be pretty rare anyway (if it's even valid?), and it wouldn't be worth making the code more messy.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020
@coolreader18
Copy link
Contributor Author

I switched to using expected_ident_found for the base error.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 4, 2020
ItemKind::Impl { .. } => return Err(err),
_ => unreachable!(),
}
impl_info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this code to a separate function in parser/diagnostics.rs?
That's where diagnostic or recovery code lives, so it's separated from the "reference parser" code describing the language.

Copy link
Contributor Author

@coolreader18 coolreader18 Dec 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I only just realized that you asked to move it to diagnostics.rs; I just moved it under recover_const_mut which is still in parser/item.rs, there's other recover_* functions there as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parser/item.rs, there's other recover_* functions there as well.

(The other function will also need to move to diagnostics.rs, but not in this PR.)

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2020
@petrochenkov
Copy link
Contributor

Could you also squash commits into one after making all the changes?

@coolreader18 coolreader18 force-pushed the recover-const-impl branch 2 times, most recently from c7dd5ff to ede6bd5 Compare December 12, 2020 20:45
@coolreader18
Copy link
Contributor Author

Alrighty, squashed.

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Dec 12, 2020

📌 Commit 1e27b65 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2020
@bors
Copy link
Contributor

bors commented Dec 13, 2020

⌛ Testing commit 1e27b65 with merge a428dd84d4e3cb80da7a395a0a047b1adf52677d...

@bors
Copy link
Contributor

bors commented Dec 13, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 13, 2020
@petrochenkov
Copy link
Contributor

LINK : fatal error LNK1201: error writing to program database 'D:\a\rust\rust\build\i686-pc-windows-msvc\test\run-make-fulldeps\relocation-model\relocation-model\foo.pdb'; check for insufficient disk space, invalid path, or insufficient privilege

Looks spurious.
@bors retry

@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 Dec 13, 2020
@bors
Copy link
Contributor

bors commented Dec 13, 2020

⌛ Testing commit 1e27b65 with merge 057937b...

@bors
Copy link
Contributor

bors commented Dec 13, 2020

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 057937b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2020
@bors bors merged commit 057937b into rust-lang:master Dec 13, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 13, 2020
@coolreader18 coolreader18 deleted the recover-const-impl branch December 13, 2020 15:50
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
Development

Successfully merging this pull request may close these issues.

5 participants