Improve parsing of access level modifier #704

omochi · 2022-09-03T05:24:53Z

This PR fix #631.
It can round-trip correctly without crashing.

Not only that, I tried to improve recovering logic here.

For example, the logic recover such wrong inputs intuitively way.

private(get, set, didSet)

See my test cases for detail.
How do you think?

CodaFi · 2022-09-04T21:10:22Z

Sources/SwiftParser/Modifiers.swift

+        afterDetail = beforeDetail
+        beforeDetail = []
+      }
+      let rparen = consume(if: .rightParen) ??


Is expect not sufficient to recover to the right paren? I would expect commas wouldn't bind as tightly as parentheses here so you wouldn't have to do the extra token munging here.

@CodaFi

I made single expect(.rightParen) version here.
Is this match with your recommendation?

https://github.com/omochi/swift-syntax/pull/1/files

This implementation is simpler.
And safer when there is no stopper token.
For example:

private( #endif

expect stops to scanning at #endif without eating many tokens following from here.

But diagnostics for

private(get, set, didSet)

is worse than before.
(see diff https://github.com/omochi/swift-syntax/pull/1/files#diff-ad19f5e399c23c3b244b7e592ced5c7b7013e224463b61962f131d4fbec70599R159-R161)

The reason of changing diagnostics is comma and right-paren have same TokenPrecedence level.
expect stops to search right-paren when see comma.
How do you think about this change of diagnostics?

If you say that previous diagnostics is better,
I try to achieve both using expect and better diagnostics.
I could be it with expectAny([.rightParen, .comma)] and some loops.

I think what @CodaFi meant was:

- var afterDetail: [RawTokenSyntax] = [] - if let _ = detail { - while !atAny([.eof, .rightParen]) { - afterDetail.append(consumeAnyToken()) - } - } else { - afterDetail = beforeDetail - beforeDetail = [] - } - let rparen = consume(if: .rightParen) ?? - RawTokenSyntax(missing: .rightParen, arena: arena) + let (unexpectedBeforeRParen, rparen) = self.expect(.rightParen)

And then use unexpectedBeforeRParen instead of afterDetail.

I did try this right now and recovery was indeed not optimal at the moment, TokenPrecedence needed some adjustments that I was planning to do anyway. I’m making them now in #707.

CodaFi · 2022-09-06T16:08:44Z

I believe Alex has improved things here such that expect should work a bit better.

omochi · 2022-09-07T07:45:23Z

@CodaFi
I updated implementation.
Please review again.
I am satisfied with all diagnostics I added in tests.

The new token precedence was useful here.
https://github.com/apple/swift-syntax/pull/704/files#diff-31d38b5d7995a4ac07a90fd878dae83e68a45f801e3ccfe213ec9ba73b6dc168R183
Thank you @ahoppen .

I tried to use only single expect without manual loop.
Unfortunately it couldn't.

For example with following, during find set keyword, starts at right of left paren,

private(get, set

expect(.rightParen) gives up at right of left paren since there are no expected token after that.
In such case, parser should move to EoF (right of set) and found set in moving.

The behavior what i want is that
seek continuously until to meet right paren or some strong token like right paren.
This is a little deferent from expect and canRecoverTo.
So I wrote manual loop control as this.
https://github.com/apple/swift-syntax/pull/704/files#diff-31d38b5d7995a4ac07a90fd878dae83e68a45f801e3ccfe213ec9ba73b6dc168R172-R180

ahoppen

Sorry, I had this review pending and apparently didn’t submit it.

Once cf4605f makes it into main, I think we should be able to search for set by calling expectContextualKeyword("set").

While we are waiting for that, I would prefer to have a single expect call. If the diagnostic is sub-optimal, could you add a FIXME to it so we can re-visit it once the extended recovery infrastructure from #718 is in main?

ahoppen · 2022-09-05T08:47:06Z

Sources/SwiftParser/Modifiers.swift

+    assert(
+      [
+        RawTokenKind.privateKeyword,
+        .fileprivateKeyword,
+        .internalKeyword,
+        .publicKeyword
+      ].contains(self.currentToken.tokenKind)
+    )


You can use atAny here:

Suggested change

assert(

[

RawTokenKind.privateKeyword,

.fileprivateKeyword,

.internalKeyword,

.publicKeyword

].contains(self.currentToken.tokenKind)

)

assert(self.atAny([.privateKeyword, .fileprivateKeyword, .internalKeyword, .publicKeyword])

ahoppen · 2022-09-05T09:39:46Z

Sources/SwiftParser/Modifiers.swift

+        afterDetail = beforeDetail
+        beforeDetail = []
+      }
+      let rparen = consume(if: .rightParen) ??


I think what @CodaFi meant was:

- var afterDetail: [RawTokenSyntax] = [] - if let _ = detail { - while !atAny([.eof, .rightParen]) { - afterDetail.append(consumeAnyToken()) - } - } else { - afterDetail = beforeDetail - beforeDetail = [] - } - let rparen = consume(if: .rightParen) ?? - RawTokenSyntax(missing: .rightParen, arena: arena) + let (unexpectedBeforeRParen, rparen) = self.expect(.rightParen)

And then use unexpectedBeforeRParen instead of afterDetail.

I did try this right now and recovery was indeed not optimal at the moment, TokenPrecedence needed some adjustments that I was planning to do anyway. I’m making them now in #707.

ahoppen · 2022-09-07T08:18:44Z

Sources/SwiftParser/Modifiers.swift

+          ),
+        detail: detail ?? RawTokenSyntax(
+          missing: .contextualKeyword,
+          text: arena.intern("set"),


arena.intern("set") is not necessary here because "set" is a StaticString that doesn’t get deallocated because it lives in the binary itself (instead of stack or heap memory). Thus, it doesn’t need to be managed by a SyntaxArena.

When I simply wrote text: "set" here,
I got following error:

SwiftSyntax/RawSyntax.swift:427: Assertion failed: token text must be managed by the arena, or known default text for the token 2022-09-08 12:00:12.816402+0900 swift-parser-test[59612:27324768] SwiftSyntax/RawSyntax.swift:427: Assertion failed: token text must be managed by the arena, or known default text for the token

It come from this assertion.

swift-syntax/Sources/SwiftSyntax/Raw/RawSyntax.swift

Lines 427 to 428 in 1754a24

assert(arena.contains(text: text) || kind.defaultText?.baseAddress == text.baseAddress,

"token text must be managed by the arena, or known default text for the token")

How do I write?

Ah, I see. Makes sense. I’ll probably remove that assert in another PR but you can just call arena.intern for now.

omochi · 2022-09-08T03:39:57Z

@ahoppen I rewrote it as single expect(.rightParen) and find set from unexpected tokens.
Diagnostics issues are marked as FIXME.

To mimic currentToken.isContextualKeyword and consume(remapping:) pattern,
I added isContextualKeyword and setter of tokenKind on RawTokenSyntax.
But I don't think that its right way.

By the way, expectContextualKeyword in cf4605f looks what I really want.
I think that we need customizability for behavior of expect and this is just expectImpl.
Can I wait for merge that?

CodaFi · 2022-09-08T22:28:08Z

@omochi Things have landed.

ahoppen

I’m sorry that this PR took so many iterations due to my recovery-related changes that would have conflicted with this one. But since it’s merged now, I think we can go ahead with this one.

Left one suggestion inline. Also, I think you need to adjust the test cases to actually have a declaration that we can attach the modifier to. Adding var x: Int or something of the sort to the test cases should do the job.

I think your helpers might also not be needed anymore.

ahoppen · 2022-09-09T13:11:25Z

Sources/SwiftParser/Modifiers.swift

+          ),
+        detail: detail ?? RawTokenSyntax(
+          missing: .contextualKeyword,
+          text: arena.intern("set"),


Ah, I see. Makes sense. I’ll probably remove that assert in another PR but you can just call arena.intern for now.

ahoppen · 2022-09-09T13:13:53Z

Sources/SwiftParser/Modifiers.swift

+      var detail: RawTokenSyntax? = nil
+      var afterDetail: [RawSyntax] = []
+
+      let (content, rightParen) = expect(.rightParen)


I’m not really a fan of taking apart the unexpected tokens here. Fortunately, with the new recovery API, you should be able to just expect the set keyword followed by a ). Setting the precedence of the set keyword to weakBracketClose will make sure that we can skip over weak punctuators (like , or :) in the details.

let unexpectedBeforeDetail: RawUnexpectedNodesSyntax? let detail: RawTokenSyntax if let setHandle = self.canRecoverToContextualKeyword("set", precedence: .weakBracketClose) { (unexpectedBeforeDetail, detail) = self.eat(setHandle) } else { unexpectedBeforeDetail = nil detail = RawTokenSyntax( missing: .contextualKeyword, text: arena.intern("set"), arena: arena ) } let (unexpectedBeforeRightParen, rightParen) = self.expect(.rightParen) details = RawDeclModifierDetailSyntax( leftParen: lparen, unexpectedBeforeDetail, detail: detail, unexpectedBeforeRightParen, rightParen: rightParen, arena: arena )

omochi

@ahoppen @CodaFi Thank you for all support for me.

My new code becomes beautiful and perfect. (really almost same as Alex suggested)
New expect utilities family is very useful and have good semantics.

I found small bug at canRecoverToContextualKeyword and fixed it.

I added var keywords to test cases keywords to run it correctly.
I saw changes for parseDeclaration.
Using canRecoverTo here is nice.

omochi · 2022-09-10T05:11:20Z

Sources/SwiftParser/Parser.swift

+    return lookahead.canRecoverTo(
+        [],
+        contextualKeywords: [name],
+        recoveryPrecedence: precedence


Unused parameter precedence is expected to pass here.

omochi · 2022-09-10T05:11:32Z

Sources/SwiftParser/Modifiers.swift

+        unexpectedBeforeDetail = nil
+        detail = RawTokenSyntax(
+          missing: .contextualKeyword,
+          text: "set",


I can use literal now. Thank you.

omochi · 2022-09-10T05:13:28Z

Tests/SwiftParserTest/Declarations.swift

+      ]
+    )
+
+    AssertParse(


It is reported input at #631.

ahoppen

Thanks for your patience. This looks great now. I’ve got one minor comment inline, otherwise this looks good to be merged.

ahoppen · 2022-09-10T05:46:10Z

Tests/SwiftParserTest/Declarations.swift

+        DiagnosticSpec(message: "Expected 'set' in modifier"),
+        DiagnosticSpec(message: "Expected ')' to end modifier"),
+        // FIXME: It should print `+` as detail of text.
+        DiagnosticSpec(message: "Unexpected text in variable")


Just FYI (not any request to change this in this PR): The unexpected text spans multiple lines here:

+ set

and that’s why we print the generic “Unexpected text” instead of “Unexpected '+ set'” (which would look ugly if there’s a line break in the diagnostic message).

I see.
I think that escaped representation is best in such case.

For example,

Unexpected '+\n '

I got an idea.
In this case, can parser split code into + and \n set ?
Currently \n is trailing trivia of +.
If they are moved to leading trivia of set,
unexpected text is only + and \n set is valid code.

If you’ve got ideas, I would be very open to discuss them in follow-up PRs. One thing to keep in mind though, is that these lines might be >100 characters long and the unexpected text might span more than 2 lines.

Also, I don’t think we should be showing \n in the diagnostic message because it’s non-trivial to read and confusing to newcomers who don’t know about the \n escape sequence.

ahoppen · 2022-09-10T05:47:46Z

Sources/SwiftParser/Modifiers.swift

+  mutating func parseAccessLevelModifier() -> RawDeclModifierSyntax {
+    let (unexpectedBeforeName, name) = expectAny(
+      [.privateKeyword, .fileprivateKeyword, .internalKeyword, .publicKeyword],
+      default: .privateKeyword


It doesn’t really matter too much but I think .internalKeyword would be the better default here because it’ Swift’s default access level if you don’t specify anything.

Ah thats right. I fixed it.

ahoppen

Looks good. Thank you!

ahoppen · 2022-09-11T06:46:43Z

@swift-ci Please test

omochi requested a review from ahoppen as a code owner September 3, 2022 05:24

CodaFi reviewed Sep 4, 2022

View reviewed changes

ahoppen mentioned this pull request Sep 5, 2022

Improve token precedence classifications #707

Merged

omochi requested review from CodaFi and removed request for ahoppen September 7, 2022 07:45

ahoppen reviewed Sep 7, 2022

View reviewed changes

omochi force-pushed the parse-access-level-modifier branch 2 times, most recently from 9e531d1 to b02d6fe Compare September 8, 2022 03:21

ahoppen reviewed Sep 9, 2022

View reviewed changes

omochi added 5 commits September 10, 2022 13:22

improve parsing of access level modifier

02073c3

use single expect

5bdb1eb

find set and eat weak tokens

65cc882

use single expect

4795006

use new expect utilities

de9e0d3

omochi force-pushed the parse-access-level-modifier branch from b02d6fe to de9e0d3 Compare September 10, 2022 05:08

omochi commented Sep 10, 2022

View reviewed changes

omochi requested review from ahoppen and removed request for CodaFi September 10, 2022 05:25

ahoppen reviewed Sep 10, 2022

View reviewed changes

Use internal for default keyword

7c8f368

omochi requested a review from ahoppen September 10, 2022 10:59

ahoppen approved these changes Sep 11, 2022

View reviewed changes

ahoppen merged commit 0be05c8 into apple:main Sep 11, 2022

omochi deleted the parse-access-level-modifier branch September 12, 2022 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve parsing of access level modifier #704

Improve parsing of access level modifier #704

omochi commented Sep 3, 2022

CodaFi Sep 4, 2022

omochi Sep 5, 2022

ahoppen Sep 5, 2022

CodaFi commented Sep 6, 2022

omochi commented Sep 7, 2022

ahoppen left a comment

ahoppen Sep 5, 2022

ahoppen Sep 5, 2022

ahoppen Sep 7, 2022

omochi Sep 8, 2022

ahoppen Sep 9, 2022

omochi commented Sep 8, 2022

CodaFi commented Sep 8, 2022

ahoppen left a comment •

edited

ahoppen Sep 9, 2022

ahoppen Sep 9, 2022

omochi left a comment •

edited

omochi Sep 10, 2022

omochi Sep 10, 2022

omochi Sep 10, 2022

ahoppen left a comment

ahoppen Sep 10, 2022

omochi Sep 10, 2022 •

edited

omochi Sep 10, 2022

ahoppen Sep 11, 2022

ahoppen Sep 10, 2022

omochi Sep 10, 2022

ahoppen left a comment

ahoppen commented Sep 11, 2022

	assert(arena.contains(text: text) \|\| kind.defaultText?.baseAddress == text.baseAddress,
	"token text must be managed by the arena, or known default text for the token")

Improve parsing of access level modifier #704

Improve parsing of access level modifier #704

Conversation

omochi commented Sep 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented Sep 6, 2022

omochi commented Sep 7, 2022

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi commented Sep 8, 2022

CodaFi commented Sep 8, 2022

ahoppen left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi Sep 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen commented Sep 11, 2022

ahoppen left a comment •

edited

omochi left a comment •

edited

omochi Sep 10, 2022 •

edited