Skip to content

Add diagnostic for canImport expression #1574

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
ahoppen merged 5 commits into from
May 8, 2023
Merged

Add diagnostic for canImport expression #1574

ahoppen merged 5 commits into from
May 8, 2023

Conversation

@StevenWong12
Copy link
Contributor

@StevenWong12 StevenWong12 commented Apr 23, 2023 •
edited
Loading

Fixes part of #1373

This PR should solve all the TODOs in testIfconfigExpr.swift.

@StevenWong12 StevenWong12 changed the title Add diagnostic for canImport expression [WIP] Add diagnostic for canImport expression Apr 23, 2023
@StevenWong12
Copy link
Contributor Author

StevenWong12 commented Apr 23, 2023 •
edited
Loading

There are 4 TODOs left in testIfconfigExpr.swift related to the value of _version, e.g.

#if canImport(A, _version: 2.2.2.2.2) // TODO: Old parser expected error on line 1: trailing components of version '2.2.2.2' are ignored
#if canImport(A, _version: >=2.2) // TODO: Old parser expected error on line 1: cannot parse module version '>=2.2'

I think we can leave these diagnostics to the type check stage, since 1) Leaving an ExprSyntax there totally makes sense; 2) We can make use of the version check tools in llvm-project (VersionTuple::tryParse) to help us generating diagnostics instead of parsing the version string on our own. What do you think? @ahoppen

kimdv
kimdv reviewed Apr 23, 2023
View reviewed changes
@StevenWong12 StevenWong12 force-pushed the canImport_diagnostic branch from e00ef9f to a0b4326 Compare April 24, 2023 06:21
ahoppen
ahoppen reviewed Apr 24, 2023
View reviewed changes
@StevenWong12 StevenWong12 force-pushed the canImport_diagnostic branch 2 times, most recently from 74cd42f to 72ed377 Compare April 26, 2023 11:14
ahoppen
ahoppen reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you, that’s much cleaner. Also I didn’t realize we already had forDirective everywhere. Using that was a good idea!

@StevenWong12 StevenWong12 force-pushed the canImport_diagnostic branch from 72ed377 to 8d538b9 Compare April 27, 2023 12:17
@StevenWong12 StevenWong12 changed the title [WIP] Add diagnostic for canImport expression Add diagnostic for canImport expression Apr 27, 2023
@StevenWong12 StevenWong12 marked this pull request as ready for review April 27, 2023 12:20
StevenWong12
StevenWong12 commented Apr 27, 2023
View reviewed changes
@StevenWong12 StevenWong12 requested a review from ahoppen April 27, 2023 12:25
ahoppen
ahoppen reviewed Apr 28, 2023
View reviewed changes
Tests/SwiftParserTest/translated/IfconfigExprTests.swift Outdated
"""#,
diagnostics: [
// TODO: Old parser expected error on line 1: _version argument cannot be empty
DiagnosticSpec(message: "cannot parse version code '\"\"'")
Copy link
Member

@ahoppen ahoppen Apr 28, 2023

Choose a reason for hiding this comment

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

Where is the code coming from here?

Copy link
Contributor Author

@StevenWong12 StevenWong12 May 2, 2023

Choose a reason for hiding this comment

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

I think the code here is because I used shortSingleLineContentDescription for the node, already removed it.

@StevenWong12 StevenWong12 force-pushed the canImport_diagnostic branch from b3f7028 to c543645 Compare May 2, 2023 16:09
@StevenWong12 StevenWong12 requested a review from ahoppen May 2, 2023 16:13
ahoppen
ahoppen reviewed May 2, 2023
View reviewed changes
@StevenWong12 StevenWong12 requested a review from ahoppen May 3, 2023 08:07
ahoppen
ahoppen reviewed May 3, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Very nice. I’ve got a few minor comments regarding documentation, otherwise this looks good to me. 👍🏽

@StevenWong12
Copy link
Contributor Author

StevenWong12 commented May 4, 2023

I've updated the documentation according to your suggestion.😀

@StevenWong12 StevenWong12 requested a review from ahoppen May 4, 2023 06:30
@kimdv
Copy link
Contributor

kimdv commented May 4, 2023

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented May 4, 2023

The test failure is unrelated to your changes @StevenWong12. The status bot is also failing with the same error. I’ll re-trigger CI once the status bot is passing.

@ahoppen
Copy link
Member

ahoppen commented May 4, 2023

@swift-ci Please test

ahoppen
ahoppen reviewed May 5, 2023
View reviewed changes
@StevenWong12 StevenWong12 force-pushed the canImport_diagnostic branch from be45c0e to 5a350d1 Compare May 5, 2023 06:40
@ahoppen
Copy link
Member

ahoppen commented May 5, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge May 5, 2023 19:41
@ahoppen
Copy link
Member

ahoppen commented May 5, 2023

@swift-ci Please test Windows

@ahoppen
Copy link
Member

ahoppen commented May 6, 2023

Looks like the PR conflicted with another one. Could rebase and resolve the conflicts?

@StevenWong12
Add diagnostic for canImport in ifconfig
0dd5df9 
Add `forDirective` parameter for `parsePrimaryExpression` to branch into directive expression parsing for `canImport`, `compiler` and `swift` keywords

Refactor version tuple parse and `VersionTuple` node and add diagnostics for it

Add assertion for the name of `Child` to make its first character uppercase
@StevenWong12
Make the description of some nodes more clear
4c23aaf 
Refactor some codes to make them cleaner
@StevenWong12
Minor changes on documentation
f372c85 
Rerun CodeGeneration
auto-merge was automatically disabled May 6, 2023 01:59

Head branch was pushed to by a user without write access

@StevenWong12 StevenWong12 force-pushed the canImport_diagnostic branch from 5a350d1 to f372c85 Compare May 6, 2023 01:59
@StevenWong12
Copy link
Contributor Author

StevenWong12 commented May 6, 2023

I've got the conflicts resolved.

@kimdv
Copy link
Contributor

kimdv commented May 7, 2023

@swift-ci please test

CodaFi
CodaFi reviewed May 7, 2023
View reviewed changes
CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift
kind: .token(choices: [.token(tokenKind: "CommaToken")]),
isOptional: true
),
Child(
Copy link
Contributor

@CodaFi CodaFi May 7, 2023

Choose a reason for hiding this comment

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

Rather than have a string of optional components, can we split the version information into its own syntax element and make that optional?

Copy link
Contributor Author

@StevenWong12 StevenWong12 May 8, 2023

Choose a reason for hiding this comment

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

That makes sense to me. I've extracted the optional part of canImport to a separate node.

@ahoppen
Copy link
Member

ahoppen commented May 8, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented May 8, 2023

@swift-ci Please test Windows

@ahoppen ahoppen enabled auto-merge May 8, 2023 17:33
@ahoppen
Copy link
Member

ahoppen commented May 8, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 8311d8d into swiftlang:main May 8, 2023
@StevenWong12 StevenWong12 mentioned this pull request Jun 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CodaFi CodaFi CodaFi left review comments

@kimdv kimdv kimdv left review comments

@ahoppen ahoppen ahoppen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@StevenWong12 @kimdv @ahoppen @CodaFi