Skip to content

Verifier: Support line numbers in fix-it verification #39572

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 31, 2022
Merged

Verifier: Support line numbers in fix-it verification #39572

merged 6 commits into from
Jan 31, 2022

Conversation

AnthonyLatsis
Copy link
Collaborator

This will come in handy for fix-its that target a different line or span multiple lines. In particular, this applies to upcoming diagnostics described in SE-0309.

@AnthonyLatsis AnthonyLatsis requested a review from xedin October 4, 2021 19:39
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test macOS

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test macOS

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test macOS

@AnthonyLatsis AnthonyLatsis requested a review from rintaro October 7, 2021 10:49
@rintaro rintaro self-assigned this Oct 8, 2021
@AnthonyLatsis
Copy link
Collaborator Author

@rintaro Friendly ping

@AnthonyLatsis
Copy link
Collaborator Author

ping

@xedin
Copy link
Contributor

xedin commented Dec 6, 2021

I have completely dropped the ball on this one, sorry! I didn't know that I was a reviewer here but since I do now, I'll take a look ASAP.

xedin
xedin approved these changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Looks good to me! @rintaro Could you please take a look as well?

@AnthonyLatsis
Copy link
Collaborator Author

@rintaro ping

@rintaro
Copy link
Member

rintaro commented Jan 28, 2022

Ooh, I'm so sorry it completely slipped my mind. I'll review it at the latest this weekend. 🙇

rintaro
rintaro approved these changes Jan 29, 2022
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

The implementation looks pretty good.

I'm not a big fan of that this only accepts absolute line numbers while @ for expected-{error|warning|note} only accepts offsets [+-]\d+, but unfortunately - is already used for the range separator, so I guess it can't be helped 😢

lib/Frontend/DiagnosticVerifier.cpp
ExpectedFixIt FixIt;
FixIt.StartLoc = OpenLoc;
FixIt.EndLoc = CloseLoc;
if (StartColStr.getAsInteger(10, FixIt.StartCol)) {
if (FirstColonLoc != StringRef::npos) {
Copy link
Member

Choose a reason for hiding this comment

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

(Not in this PR)

I feel like not specifying the line should be considered implicitly specifying the same line. Like:

if (FirstColonLoc != StringRef::npos) {
  ...
} else {
  // Implicitly on the same line as the diagnostics.
  FixIt.Range.StartLine = Expected.LineNo;
}

...

if (SecondColonLoc != StringRef::npos) {
  ...
} else {
  // Implicitly the same as the start line.
  FixIt.Range.EndLine = FixIt.Range.StartLine;
}

assert(/*verify all values are not 'NoValue' */)

As a bonus, this simplifies the check logic, right?

I'm not sure how many failures that will cause in the current test suite, and also not sure how much checking lines for all fix-its will affect the performance. Do you think it's worth to try, or perhaps did you already try it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like not specifying the line should be considered implicitly specifying the same line

I felt the same, and I also had concerns on the impact on performance, but I haven't tried. I definitely believe that diagnostics would greatly benefit from full-range fix-it verification, more so over time.

@rintaro
Copy link
Member

rintaro commented Jan 29, 2022

@swift-ci Please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci Please smoke test

rintaro
rintaro approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

LGTM!

@AnthonyLatsis
Copy link
Collaborator Author

@rintaro @xedin Thanks for reviewing, I appreciate it!

@AnthonyLatsis AnthonyLatsis merged commit 2d75d7f into swiftlang:main Jan 31, 2022
@AnthonyLatsis AnthonyLatsis deleted the fix-it-line branch January 31, 2022 09:59
@AnthonyLatsis AnthonyLatsis mentioned this pull request Feb 17, 2022
Merged
@AnthonyLatsis AnthonyLatsis mentioned this pull request Mar 21, 2022
Merged
@AnthonyLatsis AnthonyLatsis mentioned this pull request Feb 9, 2023
Merged
@AnthonyLatsis AnthonyLatsis added compiler The Swift compiler itself feature A feature request or implementation diagnostic verifier Area → compiler → Frontend: The `DiagnosticVerifier` class labels Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xedin xedin xedin approved these changes

@rintaro rintaro rintaro approved these changes

Assignees

@rintaro rintaro

Labels
compiler The Swift compiler itself diagnostic verifier Area → compiler → Frontend: The `DiagnosticVerifier` class feature A feature request or implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AnthonyLatsis @xedin @rintaro