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

Suggest fn if fun, func, function or def is used to define a function #100547

Conversation

bindsdev
Copy link
Contributor

Closes #99751

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 14, 2022
@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 @lcnr (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot
Copy link
Collaborator

rustbot commented Aug 14, 2022

The Miri submodule was changed

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2022
@rust-log-analyzer

This comment has been minimized.

@bindsdev
Copy link
Contributor Author

@lcnr: According to the CI failure message, and a couple bot messages above, it seems like I somehow modified miri. To make the CI pass and hopefully unblock the merge, what would I do to prevent the CI from failing?

@lcnr
Copy link
Contributor

lcnr commented Aug 15, 2022

you've committed a submodule change. I don't know enough about git to give you the best way to do it, but it should be easiest to use git reset HEAD~1 to undo your commit and then add all files while being careful to not readd the change to the miri submodule.

If you're still stuck I suggest asking for help with this in #compiler/help on zulip

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Aug 17, 2022

please merge your commits, this can be done using

git rebase HEAD~4 -i

and then changing pick for all but the first commit to fixup or just f.

I don't know this code enough to tell whether this would also emit this suggestion at places where it doesn't make sense, so

r? rust-lang/compiler

@rust-highfive rust-highfive assigned davidtwco and unassigned lcnr Aug 17, 2022
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

This looks good, left one comment, but you'll need to remove the merge commits and reverts and things like that, for this change, you should have only one commit with the changes you're making (if you end up with conflicts, rebase instead of merging).

compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco 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 Aug 17, 2022
@bindsdev bindsdev requested a review from davidtwco August 18, 2022 00:46
@davidtwco
Copy link
Member

This still isn't right. I'd suggest just deleting your branch locally, creating a new identically-named branch from master, applying your change again, commiting and force-pushing. That's probably going to be easier.

@bindsdev bindsdev merged commit b8c0a01 into rust-lang:master Aug 18, 2022
@bindsdev bindsdev force-pushed the akabinds/improved-invalid-function-qual-error branch from 371cb5a to 801821d Compare August 18, 2022 11:08
@rustbot rustbot added this to the 1.65.0 milestone Aug 18, 2022
@bindsdev
Copy link
Contributor Author

@davidtwco was that supposed to happen? I have never done a force push before and something feels wrong.

@davidtwco
Copy link
Member

@davidtwco was that supposed to happen? I have never done a force push before and something feels wrong.

I think it's just that you force pushed master to your branch, rather than master plus additional changes, and that made GitHub think it was merged.

@bindsdev
Copy link
Contributor Author

@davidtwco was that supposed to happen? I have never done a force push before and something feels wrong.

I think it's just that you force pushed master to your branch, rather than master plus additional changes, and that made GitHub think it was merged.

I see. It seems like no changes were recorded also. So what shall I do now?

@davidtwco
Copy link
Member

I see. It seems like no changes were recorded also. So what shall I do now?

I think you should be able to make the changes for this fix and then commit and push it.

@bindsdev
Copy link
Contributor Author

I see. It seems like no changes were recorded also. So what shall I do now?

I think you should be able to make the changes for this fix and then commit and push it.

Will I have to make another PR?

@davidtwco
Copy link
Member

I don't think so, if you add changes this should re-open, but there's no harm in another PR if that's easier too.

@bindsdev
Copy link
Contributor Author

I don't think so, if you add changes this should re-open, but there's no harm in another PR if that's easier too.

I pushed the changes about 14 hours ago. Seems like it wasn't reopened.

@davidtwco
Copy link
Member

I don't think so, if you add changes this should re-open, but there's no harm in another PR if that's easier too.

I pushed the changes about 14 hours ago. Seems like it wasn't reopened.

Feel free to open a new PR then, and include r? @davidtwco (outside of a code block) in the description so that I'm assigned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggest fn if fun, func, function or def is used instead
7 participants