Skip to content

Fixed testAvailabilityQueryUnavailability34a #1464

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

CippoX
Copy link
Contributor

@CippoX CippoX commented Mar 29, 2023

I'm clearly doing something wrong because Source Control keeps breaking down. Problems aside, I've implemented the changes you suggested and now it looks way better. Unfortunately I am still having trouble fixing this edge case
if #available(*) == false && other_condition

@CippoX CippoX requested a review from ahoppen as a code owner March 29, 2023 12:54
@CippoX
Copy link
Contributor Author

CippoX commented Mar 31, 2023

@ahoppen I've fixed everything you suggested. However, the fact that I had to introduce two new elements getTokenRange and ReplaceWithMultipleTokensFixIt, looks odd to me. Is there a better way?

@CippoX
Copy link
Contributor Author

CippoX commented Mar 31, 2023

The only way I see to fix this specific case if #available(*) == false && other_condition, is to assign unexpectedBeforeKeepGoing to another variable like

public struct RawConditionElementSyntax: RawSyntaxNodeProtocol {
  ...
  public var unexpectedInsteadOfTrailingComma: RawUnexpectedNodesSyntax? {...}
  ...
}

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch 2 times, most recently from 6367e1c to e7dc9a4 Compare April 5, 2023 15:30
@CippoX
Copy link
Contributor Author

CippoX commented Apr 5, 2023

If you don't mind, I would like to resolve the visit(_ node: ForInStmtSyntax) FIXME in another PR.

@CippoX CippoX requested a review from ahoppen April 5, 2023 15:35
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. The PR looks veery good now. I’ve got three local comments but they should be straightforward to fix. After this we can merge it. 🎉

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch from af96c05 to 325f869 Compare April 6, 2023 10:00
@CippoX CippoX requested a review from ahoppen April 6, 2023 10:00
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 for improving the diagnostic. This looks good.

@ahoppen
Copy link
Member

ahoppen commented Apr 6, 2023

@CippoX Could you rebase your PR onto main. There appears is a merge conflict.

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch from 325f869 to f602144 Compare April 6, 2023 16:53
@CippoX
Copy link
Contributor Author

CippoX commented Apr 6, 2023

@ahoppen now everything should be ok 👍

@ahoppen
Copy link
Member

ahoppen commented Apr 6, 2023

Thanks. If CI is happy, I’ll merge it.

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Apr 6, 2023

CI failed in one test case, I was curious what it was, debugged it and this diff should fix the problem. Could you apply it to your PR?

diff --git a/Sources/SwiftParserDiagnostics/MissingNodesError.swift b/Sources/SwiftParserDiagnostics/MissingNodesError.swift
index 9e17832a6..7faffba2d 100644
--- a/Sources/SwiftParserDiagnostics/MissingNodesError.swift
+++ b/Sources/SwiftParserDiagnostics/MissingNodesError.swift
@@ -112,13 +112,17 @@ func nodesDescription<SyntaxType: SyntaxProtocol>(_ nodes: [SyntaxType], format:
 func nodesDescriptionAndCommonParent<SyntaxType: SyntaxProtocol>(_ nodes: [SyntaxType], format: Bool) -> (commonAncestor: Syntax?, description: String) {
   let missingSyntaxNodes = nodes.map(Syntax.init)
 
-  // If all tokens in the parent are missing, return the parent type name.
-  if let commonAncestor = findCommonAncestor(missingSyntaxNodes),
+  // If all tokens in the parent are missing, return the parent type name unless
+  // we are replacing by a single token that has explicit text, in which case we
+  // return that explicit text.
+  if !isIdentifierWithCustomName,
+    let commonAncestor = findCommonAncestor(missingSyntaxNodes),
     commonAncestor.isMissingAllTokens,
     let firstToken = commonAncestor.firstToken(viewMode: .all),
     let lastToken = commonAncestor.lastToken(viewMode: .all),
     missingSyntaxNodes.contains(Syntax(firstToken)),
-    missingSyntaxNodes.contains(Syntax(lastToken))
+    missingSyntaxNodes.contains(Syntax(lastToken)),
+    nodes.only?.as(TokenSyntax.self)?.text != nil
   {
     if let nodeTypeName = commonAncestor.nodeTypeNameForDiagnostics(allowBlockNames: true) {
       return (commonAncestor, nodeTypeName)

@CippoX
Copy link
Contributor Author

CippoX commented Apr 6, 2023

sure

@CippoX
Copy link
Contributor Author

CippoX commented Apr 6, 2023

@ahoppen where is isIdentifierWithCustomName defined?

@ahoppen
Copy link
Member

ahoppen commented Apr 6, 2023

Oh, sorry, copy-pasted the wrong diff. It should have been

diff --git a/Sources/SwiftParserDiagnostics/MissingNodesError.swift b/Sources/SwiftParserDiagnostics/MissingNodesError.swift
index 9e17832a6..7faffba2d 100644
--- a/Sources/SwiftParserDiagnostics/MissingNodesError.swift
+++ b/Sources/SwiftParserDiagnostics/MissingNodesError.swift
@@ -112,13 +112,17 @@ func nodesDescription<SyntaxType: SyntaxProtocol>(_ nodes: [SyntaxType], format:
 func nodesDescriptionAndCommonParent<SyntaxType: SyntaxProtocol>(_ nodes: [SyntaxType], format: Bool) -> (commonAncestor: Syntax?, description: String) {
   let missingSyntaxNodes = nodes.map(Syntax.init)
 
  if let commonAncestor = findCommonAncestor(missingSyntaxNodes),
-  // If all tokens in the parent are missing, return the parent type name.
+  // If all tokens in the parent are missing, return the parent type name unless
+  // we are replacing by a single token that has explicit text, in which case we
+  // return that explicit text.
     commonAncestor.isMissingAllTokens,
     let firstToken = commonAncestor.firstToken(viewMode: .all),
     let lastToken = commonAncestor.lastToken(viewMode: .all),
     missingSyntaxNodes.contains(Syntax(firstToken)),
-    missingSyntaxNodes.contains(Syntax(lastToken))
+    missingSyntaxNodes.contains(Syntax(lastToken)),
+    nodes.only?.as(TokenSyntax.self)?.text != nil
   {
     if let nodeTypeName = commonAncestor.nodeTypeNameForDiagnostics(allowBlockNames: true) {
       return (commonAncestor, nodeTypeName)

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch from ba7db24 to f47c7b2 Compare April 6, 2023 21:30
@CippoX
Copy link
Contributor Author

CippoX commented Apr 6, 2023

All done, and thanks for debugging it!

@ahoppen
Copy link
Member

ahoppen commented Apr 6, 2023

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member

ahoppen commented Apr 7, 2023

@swift-ci Please test

@CippoX
Copy link
Contributor Author

CippoX commented Apr 7, 2023

Is testInvalid29 the test that was causing the issue? Because if I understood the log correctly and that's the problem, the bug seems to be caused by the way I've changed the ReplaceTokensFixIt function.

So I suggest checking if replacements contains one element and then providing the appropriate FixIt. The alternative would be restoring the old function and introducing a new function that takes multiple replacements.

@ahoppen
Copy link
Member

ahoppen commented Apr 7, 2023

Is testInvalid29 the test that was causing the issue? Because if I understood the log correctly and that's the problem, the bug seems to be caused by the way I've changed the ReplaceTokensFixIt function.

Yes, correct.

So I suggest checking if replacements contains one element and then providing the appropriate FixIt. The alternative would be restoring the old function and introducing a new function that takes multiple replacements.

I thought that’s essentially what I did with my diff but looks like I was wrong and didn’t run all tests to verify, sorry about that. I don’t think we should add a new function if you could investigate in finding some way to make the current one behave in both scenarios, that’d be great.

@CippoX
Copy link
Contributor Author

CippoX commented Apr 7, 2023

@ahoppen I've pushed my idea, let me know

@CippoX
Copy link
Contributor Author

CippoX commented Apr 7, 2023

I know my solution is very amateur, but it looks quite hard to change nodesDescriptionAndCommonParent without breaking something else.

@ahoppen
Copy link
Member

ahoppen commented Apr 11, 2023

I just looked into it further and it looks like my previous solution was really close, I just forgot to check for empty string instead of nil. The following diff should get all tests passing and has a generic implementation in nodesDescriptionAndCommonParent

diff --git a/Sources/SwiftParserDiagnostics/MissingNodesError.swift b/Sources/SwiftParserDiagnostics/MissingNodesError.swift
index b5f1215e..3e6b9f34 100644
--- a/Sources/SwiftParserDiagnostics/MissingNodesError.swift
+++ b/Sources/SwiftParserDiagnostics/MissingNodesError.swift
@@ -112,6 +112,13 @@ func nodesDescription<SyntaxType: SyntaxProtocol>(_ nodes: [SyntaxType], format:
 func nodesDescriptionAndCommonParent<SyntaxType: SyntaxProtocol>(_ nodes: [SyntaxType], format: Bool) -> (commonAncestor: Syntax?, description: String) {
   let missingSyntaxNodes = nodes.map(Syntax.init)
 
+  let isOnlyTokenWithNonMissingText: Bool
+  if let token = nodes.only?.as(TokenSyntax.self) {
+    isOnlyTokenWithNonMissingText = token.text != ""
+  } else {
+    isOnlyTokenWithNonMissingText = false
+  }
+
   // If all tokens in the parent are missing, return the parent type name unless
   // we are replacing by a single token that has explicit text, in which case we
   // return that explicit text.
@@ -120,7 +127,8 @@ func nodesDescriptionAndCommonParent<SyntaxType: SyntaxProtocol>(_ nodes: [Synta
     let firstToken = commonAncestor.firstToken(viewMode: .all),
     let lastToken = commonAncestor.lastToken(viewMode: .all),
     missingSyntaxNodes.contains(Syntax(firstToken)),
-    missingSyntaxNodes.contains(Syntax(lastToken))
+    missingSyntaxNodes.contains(Syntax(lastToken)),
+    !isOnlyTokenWithNonMissingText
   {
     if let nodeTypeName = commonAncestor.nodeTypeNameForDiagnostics(allowBlockNames: true) {
       return (commonAncestor, nodeTypeName)
diff --git a/Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift b/Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift
index 52198c2b..fd2af952 100644
--- a/Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift
+++ b/Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift
@@ -614,10 +614,6 @@ public struct ReplaceTokensFixIt: ParserFixIt {
   public let replacements: [TokenSyntax]
 
   public var message: String {
-    if replacements.count == 1 {
-      return "replace \(nodesDescription(replaceTokens, format: false)) with '\(replacements[0].text)'"
-    } else {
-      return "replace \(nodesDescription(replaceTokens, format: false)) with \(nodesDescription(replacements, format: false))"
-    }
+    return "replace \(nodesDescription(replaceTokens, format: false)) with \(nodesDescription(replacements, format: false))"
   }
 }

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch from 2a9caa7 to a0fc474 Compare April 12, 2023 10:19
@CippoX
Copy link
Contributor Author

CippoX commented Apr 12, 2023

Thanks!

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. This looks great now!

@ahoppen
Copy link
Member

ahoppen commented Apr 12, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge April 12, 2023 19:54
@ahoppen
Copy link
Member

ahoppen commented Apr 12, 2023

@swift-ci Please test Windows

@ahoppen
Copy link
Member

ahoppen commented Apr 13, 2023

Looks like this PR raced with the one that changed replacement to replacements (I can’t find it right now). Could you rebase and make sure that everything builds locally?

auto-merge was automatically disabled April 13, 2023 15:04

Head branch was pushed to by a user without write access

@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

@swift-ci please test

@CippoX
Copy link
Contributor Author

CippoX commented Apr 13, 2023

@kimdv please wait, looks there's something wrong 🥲

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch from 7e927aa to 5457d5f Compare April 13, 2023 15:24
@CippoX
Copy link
Contributor Author

CippoX commented Apr 13, 2023

Honestly, I'm not sure what happened, but everything seems to be fine now. I've also removed a warning introduced by my previous PR.

@CippoX CippoX force-pushed the fixing-available-cannot-be-used-as-an-expression branch from 5457d5f to 07fff06 Compare April 13, 2023 15:35
@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

@swift-ci please test

@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

Let's see what the CI says 🤞

@ahoppen
Copy link
Member

ahoppen commented Apr 13, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge April 13, 2023 18:02
@ahoppen ahoppen merged commit 357a3e6 into swiftlang:main Apr 13, 2023
@CippoX
Copy link
Contributor Author

CippoX commented Apr 13, 2023

Finally 🎉

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 14, 2023
These parser failures were found by swiftlang#1340.

- There was one round-trip failure introduced by swiftlang#1464
- A few cases were just consume tokens as the wrong child
- The list of expected token kinds of `DeclModifierSynax` didn’t contain one modifier that was actually parsed
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 14, 2023
These parser failures were found by swiftlang#1340.

- There was one round-trip failure introduced by swiftlang#1464
- A few cases were just consume tokens as the wrong child
- The list of expected token kinds of `DeclModifierSynax` didn’t contain one modifier that was actually parsed
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.

3 participants