Skip to content

[FixIt]: Diagnostic for use of plain protocol name in type position suggests some or any #74197

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

Conversation

saehejkang
Copy link
Contributor

Changes Made

  • Logic added to add two new fixits when appropriate
  • Updated existing error message to add the some keyword
  • Added two new notes recommending replacements
  • Updated test files

Resolves #68284

@saehejkang saehejkang marked this pull request as draft June 7, 2024 05:11
@saehejkang saehejkang marked this pull request as ready for review June 11, 2024 02:12
@saehejkang
Copy link
Contributor Author

marking as ready for an intial review

@AnthonyLatsis
Copy link
Collaborator

Thanks for the reminder, and please do not hesitate to ping me or other code owners once every 2 weeks if your PR stagnates.

Have you run any relevant tests locally? If not, please do in subsequent rounds. As a first step, I am going to run a smoke test for you. Shall it fail, please find the test failures in the console output of the job run and try to address them accordingly. Reach out any time.

P.S. What happened to the previous PR?

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test Linux

@saehejkang
Copy link
Contributor Author

saehejkang commented Aug 16, 2024

Have you run any relevant tests locally?

I think I ran the tests locally a while ago and it passed. The command I ran is below:

../llvm-project/llvm/utils/lit/lit.py -s -vv \
  ../build/Ninja-RelWithDebInfoAssert/swift-macosx-$(uname -m)/test-macosx-$(uname -m) \
  --filter="protocol.swift"

Not sure if that is enough/what is needed to be ran.

EDIT: I just ran the command below and am getting the same errors as the jenkins build.

utils/run-test --lit ../llvm-project/llvm/utils/lit/lit.py \
  ../build/Ninja-RelWithDebInfoAssert/swift-macosx-$(uname -m)/test-macosx-$(uname -m) \
  --filter="protocol.swift"

Please find the test failures in the console output of the job run and try to address them accordingly.

From the jenkins logs, the error is here. It has to do with pieces of code that I did not touch at all, so I am not sure why that fails. Could it be because my branch is out of date from main? Looks like it never even gets to running the tests that I updated.

P.S. What happened to the previous PR?

I think I messed up the branch and ended up pushing to my main branch instead of a feature branch. I tried to fix it but accidentally removed all the commits and the PR automatically closed.

@saehejkang
Copy link
Contributor Author

@AnthonyLatsis ping

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test Linux

@saehejkang
Copy link
Contributor Author

saehejkang commented Sep 29, 2024

I checked out main and pulled the latest code as well as ran utils/update-checkout. I merged main into this feature branch to update everything. However, when I re run the build on the main branch, I get no errors. I seem to get errors when running updates on this feature branch and can't understand why. Usually running an entire clean build works, but it isn't anymore.

swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/stdlib/public/core/OSX/arm64/Swift.o is having issues

@AnthonyLatsis
Copy link
Collaborator

swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/stdlib/public/core/OSX/arm64/Swift.o is having issues

That must be the failure from #74197 (comment) (an assertion failure when building the standard library).

@saehejkang
Copy link
Contributor Author

saehejkang commented Nov 4, 2024

@AnthonyLatsis any tips on squashing commits? When I want to run git rebase head~5 it shows a lot of old commits from the proeject so I can't find the right ones to squash.

@AnthonyLatsis
Copy link
Collaborator

When I want to run git rebase head~5 it shows a lot of old commits from the project so I can't find the right ones to squash.

This means HEAD~5 was neither one of your feature branch commits nor the commit that your branch is cut off from. Always make sure to not rebase and, thus, overwrite merged commits. Note that HEAD~N is not necessarily the N + 1th commit from git log. Personally, I like to run git log --oneline, find the commit that I want to rebase the rest onto, and pass its hash to git rebase -i, just to be sure that I am not about to make a mess.

You were doing an interactive rebase (-i), right? Did you manage to abort it?

@saehejkang
Copy link
Contributor Author

saehejkang commented Nov 5, 2024

You were doing an interactive rebase (-i), right? Did you manage to abort it?

Yes, rebase -i is what I did and I did manage to abort it.


Wouldn't the hash I want to rebase the rest onto be 0bc3a04ed4f36c72b9a59e94fa283c2587279535 (the last commit in this PR)?

@AnthonyLatsis
Copy link
Collaborator

AnthonyLatsis commented Nov 5, 2024

To rebase basically means to replay a commit range on top of a base commit. A rebase will never modify the original commit range. It will replay clones instead. The beauty of the interactive mode is that it allows one to act upon the commits being replayed, such as to edit or squash them. This is how you rewrite history. The general form is:

git rebase --onto <new_base> <from> <to> 

The range is half-open (this is important to keep in mind): (<from>, <to>]. <to> defaults to the current branch, <new_base> defaults to <from>.

Now to the practical part. Consider the following tree:

          D---E---F feature (HEAD)
         /
A---B---C main

We want to squash E and F into D and make the tree look like so:

          D' feature (HEAD)
         /
A---B---C main

This can be done by interactively rebasing (main, feature] or (C, feature] onto main or C, and editing the todo list to squash E and F into D:

# or 'git rebase -i main' for short. 
git rebase -i --onto main main feature
pick D
squash E
squash F

Replacing main with C in the above command will make no difference, but note that passing F instead of feature will not cause the feature ref to be updated. It would still point to F. You would end up with the following state instead:

         D' (detached HEAD)
        /
        | D---E---F feature
        |/
A---B---C main

This is a good occasion to recall that a rebase does not touch the original commits. I simply omitted them in the previous diagram, where feature no longer points to F.

@AnthonyLatsis
Copy link
Collaborator

I forgot you have merge commits in the mix. Merge commits make rebasing a lot more nuanced. Please drop them before squashing (you should be able to do so by running git rebase main from your branch). Merge commits are not generally accepted into the base repository anyway. You should rebase feature branches on top of an updated main instead.

@saehejkang saehejkang force-pushed the plain-protocol-suggest-some-or-any branch from 0bc3a04 to 86adffa Compare November 7, 2024 02:57
@saehejkang
Copy link
Contributor Author

ready for a review again!! @AnthonyLatsis

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test macOS

@AnthonyLatsis
Copy link
Collaborator

@saehejkang Time to debug some failures. Sorry for the hold-up.

Hint: The current change is purely additive. Also, the condition you specify for the new diagnostic is looser than the condition for the old one.

@saehejkang
Copy link
Contributor Author

Also, the condition you specify for the new diagnostic is looser than the condition for the old one.

I assume I am missing a check for isAnyOrSomeMissing()

@AnthonyLatsis
Copy link
Collaborator

You are, but the current diagnosis is already wrapped in an appropriate if. Why a second one?

@saehejkang
Copy link
Contributor Author

closing as I am no longer working on the issue

@saehejkang saehejkang closed this Mar 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diagnostic for use of plain protocol name in type position should suggest some or any
2 participants