Add diagnostics for missing modifier in operator declaration #1448

StevenWong12 · 2023-03-26T04:18:48Z

Part of #1373

Tests/SwiftParserTest/translated/OperatorDeclTests.swift

StevenWong12 · 2023-03-27T08:37:27Z

Sources/SwiftParser/Declarations.swift

@@ -1964,9 +1964,16 @@ extension Parser {
    } else {
      unexpectedAtEnd = nil
    }
+
+    var modifiers: RawModifierListSyntax? = attrs.modifiers
+    if modifiers == nil, unexpectedBeforeOperatorKeyword == nil {


I check the unexpectedBeforeOperatorKeyword here to keep testOperatorDecl10 produce the same diagnostics as the old parser. Not sure whether this makes sense🤔

ahoppen

I haven’t looked at your changes in detail yet but I think the way to fix this is to change how OperatorDecl is represented. It shouldn’t have an arbitrary ModifierList but instead require a single child that can be either of the following tokens: prefix, infix, postfix. And then parseOperatorDeclaration wouldn’t actually be passed a DeclAttributes (which is essentially attributes + modifiers). Instead, it would require some handling in parseDecl earlier on.

Does this make sense to you?

StevenWong12 · 2023-03-29T15:38:05Z

My changes just handled the case that missing modifier for operator declaration. Your comments reminded me that the wrong modifier here is also not handled yet and I have updated the codes. 😀

As for the implementation, I prefer to parse the modifier list in parseDecl and handle the misused or missing modifiers in parseOperatorDeclaration. Then we could leave other work to ParseDiagnosticsGenerator.

StevenWong12 · 2023-03-29T15:41:22Z

Tests/SwiftParserTest/translated/OperatorDeclTests.swift

+    prefix 1️⃣postfix operator ** : PrecedenceGroup
+    """,
+    diagnostics: [
+        DiagnosticSpec(message: "unexpected code 'postfix' in operator declaration")


The old parser generates 'postfix' contradicts previous modifier 'prefix' here. If my changes work, I could optimize the diagnostics message here later.

ahoppen · 2023-03-30T15:40:44Z

As for the implementation, I prefer to parse the modifier list in parseDecl and handle the misused or missing modifiers in parseOperatorDeclaration. Then we could leave other work to ParseDiagnosticsGenerator.

I disagree. With the current design, if you are given an arbitrary SwiftSyntax tree, nothing guarantees you that an operator declaration has a prefix, infix or postfix modifier. So all clients of SwiftSyntax would need to handle the case that these are missing since the syntax tree could have been constructed by something other than the parser (e.g. SwiftSyntaxBuilder).

StevenWong12 · 2023-04-02T06:37:29Z

I have refactored how OperatorDecl is presented, with a TokenSyntax to represent infix, prefix or postfix and get it checked in RawSyntaxValidation.swift.

According to the doc, we can not have attributes in operator declarations, so I have also removed the attributes in OperatorDeclSyntax.

As for SwiftParser,

Instead, it would require some handling in parseDecl earlier on

I think handling in the early stage of parseDecl will require an additional call of canRecoverTo to help us determine whether this declaration needs DeclAttributes, which will bring some unnecessary overhead, since only operator declaration and precedence group declaration do not need DeclAttributes.

So I make the attributes and modifiers list parsed as before, and get them handled in parseOperatorDeclarationIntroducer. Does it make sense to you? @ahoppen

If there is anything I missed, please feel free to let me know.

Thanks in advance!

Tests/SwiftParserTest/translated/OperatorDeclTests.swift

Sources/SwiftParser/Declarations.swift

StevenWong12 · 2023-04-06T13:42:20Z

You would just need to make sure that we call into parseOperatorDeclaration before the attributes are getting parsed in parseDeclaration

To achieve this, I made a most straightforward way: make an extra call of canRecoverTo. This indeed makes the parseDeclaration and parseOperatorDecl more simple.

Considering I call the extra canRecoverTo before parseAttributeList and parseModifierList, I add two extra test cases (testOperatorDecl124 and testOperatorDecl125) to make sure the output fall into expectation.

Also, since I call the extra canRecoverTo every time when we parse a declaration, I wonder whether the performance loss caused by it is acceptable?

ahoppen

Yes, this looks much simpler now. I’ve got a few local comments inline, after that it’s ready to be merged.

Sources/SwiftParser/Declarations.swift

Sources/SwiftParser/Lookahead.swift

Sources/SwiftParser/SyntaxUtils.swift

StevenWong12 · 2023-04-06T17:50:12Z

Sorry for committing some test codes before. I have already removed them.😁

ahoppen

Thanks. Let’s get this merged.

ahoppen · 2023-04-06T17:56:10Z

@swift-ci Please test

ahoppen · 2023-04-06T20:32:59Z

https://github.com/apple/swift/blob/main/test/attr/accessibility.swift failed to round-trip in the parser. Could you investigate what’s going wrong here? A good place to start is

Clone https://github.com/apple/swift
Build swift-parser-cli in this repo
Run swift-parser-cli reduce path/to/test/attr/accessibility.swift. That should give you a reduced test case
Create an assertParse test case from the output of the step above and fix that.

StevenWong12 · 2023-04-07T07:05:51Z

I think the problem lies in that private modifier has declKeyword precedence so that we can not recover to operator until modifiers are parsed.

My solution is to add a new declModifier token precedence group whose priority just lower than declKeyword. Also, I promote the token precedence of operator modifiers when we try to recover to them and exclude other modifiers.

Actually, this problem is supposed to be detected by testOperatorDecl125 but I find that the recovery precedence of mutating keyword is not explicitly assigned 😂 .

kimdv · 2023-04-07T08:35:38Z

@swift-ci please test

ahoppen · 2023-05-25T18:34:47Z

Oh, and two more things: Looks like there is a merge conflict and can you squash your commits and force push? It makes for a nicer commit history IMO.

StevenWong12 · 2023-05-26T06:14:35Z

Done!

ahoppen · 2023-05-26T20:18:37Z

@swift-ci Please test

ahoppen · 2023-05-26T20:19:01Z

@swift-ci Please test Windows

ahoppen · 2023-05-26T20:19:51Z

@swift-ci Please test Windows

ahoppen · 2023-05-26T21:26:47Z

Sorry @StevenWong12, looks like introduced another conflict by merging #1668. Could you rebase one more time?

StevenWong12 · 2023-05-29T05:21:28Z

Done!

I noticed that there is a regression test(SwiftFormatPrettyPrintTests.OperatorDeclTests) in swift-format failed and I opened a pr to sync this change.

kimdv · 2023-05-30T05:54:38Z

swiftlang/swift-format#536

@swift-ci please test

StevenWong12 · 2023-05-30T11:21:15Z

Could you please trigger the test on Windows? @kimdv

kimdv · 2023-05-30T11:38:56Z

swiftlang/swift-format#536

@swift-ci please test Windows

ahoppen · 2023-05-30T18:45:42Z

And another merge conflict, this time from #1711. Sorry that you continue to need to rebase this PR.

kimdv · 2023-05-30T20:24:53Z

When rebased I'll merge tomorrow if it's not merged before or if it's not okay @ahoppen 😁

Assign precedence to most of declaration modifiers Add diagnostic for missing fixity in operator declaration Make `unowned` parsing accept `RecoveryConsumptionHandle`

StevenWong12 · 2023-05-31T05:39:40Z

That's not a big deal. 😄

Already rebased.

kimdv · 2023-05-31T06:31:48Z

@swift-ci please test

kimdv · 2023-05-31T06:32:05Z

@swift-ci please test windows

StevenWong12 · 2023-05-31T08:27:24Z

I think maybe we should include the pr(swiftlang/swift-format#536) in swift-format to trigger the test like yesterday.

Could you please trigger that again? @kimdv

kimdv · 2023-05-31T08:28:20Z

swiftlang/swift-format#536

@swift-ci please test

kimdv · 2023-05-31T08:28:36Z

I think maybe we should include the pr(apple/swift-format#536) in swift-format to trigger the test like yesterday.

Could you please trigger that again? @kimdv

Doh! Sorry about that.
Triggered it again

StevenWong12 · 2023-05-31T08:29:14Z

Thank you. 😁

StevenWong12 force-pushed the operator_decl_diagnostics branch from 7da6fdf to 7d5684e Compare March 26, 2023 12:18

StevenWong12 commented Mar 26, 2023

View reviewed changes

Tests/SwiftParserTest/translated/OperatorDeclTests.swift Outdated Show resolved Hide resolved

StevenWong12 force-pushed the operator_decl_diagnostics branch from 7d5684e to 0a98797 Compare March 27, 2023 08:33

StevenWong12 commented Mar 27, 2023

View reviewed changes

StevenWong12 marked this pull request as ready for review March 27, 2023 08:42

StevenWong12 requested a review from ahoppen as a code owner March 27, 2023 08:42

ahoppen reviewed Mar 27, 2023

View reviewed changes

StevenWong12 commented Mar 29, 2023

View reviewed changes

StevenWong12 force-pushed the operator_decl_diagnostics branch from 91c5978 to c0c5647 Compare March 29, 2023 15:45

StevenWong12 requested a review from ahoppen March 29, 2023 15:49

StevenWong12 force-pushed the operator_decl_diagnostics branch from c0c5647 to d7c4d4b Compare April 2, 2023 06:19

StevenWong12 requested a review from DougGregor as a code owner April 2, 2023 06:19

ahoppen reviewed Apr 4, 2023

View reviewed changes

Tests/SwiftParserTest/translated/OperatorDeclTests.swift Outdated Show resolved Hide resolved

Sources/SwiftParser/Declarations.swift Show resolved Hide resolved

StevenWong12 force-pushed the operator_decl_diagnostics branch from dfaa2bd to 5cb2987 Compare April 6, 2023 13:37

StevenWong12 requested a review from ahoppen April 6, 2023 13:42

ahoppen reviewed Apr 6, 2023

View reviewed changes

Sources/SwiftParser/Declarations.swift Outdated Show resolved Hide resolved

Sources/SwiftParser/Lookahead.swift Outdated Show resolved Hide resolved

Sources/SwiftParser/SyntaxUtils.swift Outdated Show resolved Hide resolved

StevenWong12 force-pushed the operator_decl_diagnostics branch from 5cb2987 to 8aa865d Compare April 6, 2023 17:49

StevenWong12 requested a review from ahoppen April 6, 2023 17:50

ahoppen previously approved these changes Apr 6, 2023

View reviewed changes

StevenWong12 requested a review from ahoppen April 7, 2023 07:09

StevenWong12 force-pushed the operator_decl_diagnostics branch from 3c8d46e to 5300fa7 Compare April 7, 2023 16:18

StevenWong12 force-pushed the operator_decl_diagnostics branch from 869eafb to 7a7c56a Compare May 25, 2023 16:09

StevenWong12 force-pushed the operator_decl_diagnostics branch from 7a7c56a to cf641d8 Compare May 26, 2023 06:13

ahoppen approved these changes May 26, 2023

View reviewed changes

ahoppen enabled auto-merge May 26, 2023 20:18

auto-merge was automatically disabled May 29, 2023 05:15
Head branch was pushed to by a user without write access

StevenWong12 force-pushed the operator_decl_diagnostics branch from cf641d8 to 1dc5a41 Compare May 29, 2023 05:15

StevenWong12 mentioned this pull request May 29, 2023

Add Token.break after fixity in operator declaration swiftlang/swift-format#536

Merged

Change how OperatorDeclSyntax is presented

dbf2a8b

Assign precedence to most of declaration modifiers Add diagnostic for missing fixity in operator declaration Make `unowned` parsing accept `RecoveryConsumptionHandle`

StevenWong12 force-pushed the operator_decl_diagnostics branch from 1dc5a41 to dbf2a8b Compare May 31, 2023 05:39

kimdv merged commit 6941aa6 into swiftlang:main May 31, 2023

StevenWong12 mentioned this pull request Jun 10, 2023

Add Fix-Its for canImport diagnose #1765

Closed

Add diagnostics for missing modifier in operator declaration #1448

Add diagnostics for missing modifier in operator declaration #1448

Uh oh!

Conversation

StevenWong12 commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

StevenWong12 Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

StevenWong12 commented Mar 29, 2023

Uh oh!

StevenWong12 Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StevenWong12 commented Apr 2, 2023

Uh oh!

Uh oh!

Uh oh!

StevenWong12 commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StevenWong12 commented Apr 6, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Apr 6, 2023

Uh oh!

ahoppen commented Apr 6, 2023

Uh oh!

StevenWong12 commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimdv commented Apr 7, 2023

Uh oh!

ahoppen commented May 25, 2023

Uh oh!

StevenWong12 commented May 26, 2023

Uh oh!

ahoppen commented May 26, 2023

Uh oh!

ahoppen commented May 26, 2023

Uh oh!

ahoppen commented May 26, 2023

Uh oh!

ahoppen commented May 26, 2023

Uh oh!

StevenWong12 commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimdv commented May 30, 2023

Uh oh!

StevenWong12 commented May 30, 2023

Uh oh!

kimdv commented May 30, 2023

Uh oh!

ahoppen commented May 30, 2023

Uh oh!

kimdv commented May 30, 2023

Uh oh!

StevenWong12 commented May 31, 2023

Uh oh!

kimdv commented May 31, 2023

Uh oh!

kimdv commented May 31, 2023

Uh oh!

StevenWong12 commented Mar 26, 2023 •

edited

Loading

ahoppen commented Mar 30, 2023 •

edited

Loading

StevenWong12 commented Apr 6, 2023 •

edited

Loading

StevenWong12 commented Apr 7, 2023 •

edited

Loading

StevenWong12 commented May 29, 2023 •

edited

Loading