Add assertion for missing fixed source #1549

kimdv · 2023-04-14T22:11:55Z

As in #1467 I also tough here it would start to make sense to test the applied the fix its as it will ensure the quality in many different cases and scenarios.

So when there is 1 or more fix its we expect a fixed source.
What do you think. Is it worth it?

Just before I continue with the rest of the 195 failing test cases 🫣

ahoppen · 2023-04-14T22:30:49Z

As with my comment on the other PR, I think that it’s a great change and allows everybody to explicitly write down their expectation. I certainly remember a bug or two that would have been caught if we had written down the fixed source straight away.

Tests/SwiftParserTest/Assertions.swift

Tests/SwiftParserTest/AttributeTests.swift

kimdv

A bit less than 150 missing fix its. 😬
Added some online comments that could be worth considering

kimdv · 2023-04-15T08:26:45Z

Tests/SwiftParserTest/AttributeTests.swift

+      fixedSource: """
+      @_expose(Cxx, ""baz) func foo() {}


This looks odd. Should the fixed source not be like @_expose(Cxx, "baz") func foo() {}

kimdv · 2023-04-15T08:27:10Z

Tests/SwiftParserTest/AttributeTests.swift

+      fixedSource: """
+      @_unavailableFromAsync(messagenope: "abc")
+      func foo() {}
+      """


Should the attribute not be @_unavailableFromAsync(message: "abc")

kimdv · 2023-04-15T08:27:41Z

Tests/SwiftParserTest/AttributeTests.swift

+      fixedSource: """
+      @_unavailableFromAsync(message: ""= "abc")
+      func foo() {}
+      """


Should the attribute not be @_unavailableFromAsync(message: "abc")

kimdv · 2023-04-15T08:28:29Z

Tests/SwiftParserTest/AttributeTests.swift

+      @_unavailableFromAsync(message: ""abc)
+      func foo() {}


Same as above, I would expect that abc got wrapped in the ""

kimdv · 2023-04-15T08:28:59Z

Tests/SwiftParserTest/translated/ActorTests.swift

+        actor MyActor2 {
+            init() {
+            }
+        func hello() { }}


Should we here add the } on a new line?

kimdv · 2023-04-15T18:01:49Z

Tests/SwiftParserTest/DeclarationTests.swift

-      ]
+      ],
+      fixedSource: """
+      func foo(first second: third) struct <#identifier#>: Int) {}


I think we miss a ; between the function and struct

kimdv · 2023-04-15T18:02:28Z

Tests/SwiftParserTest/DeclarationTests.swift

-      ]
+      ],
+      fixedSource: """
+      func foo(first second: [third] fourth: Int) {}


Maybe a good first issue to add a comma after [third]

kimdv · 2023-04-17T09:28:49Z

Tests/SwiftParserTest/translated/OriginalDefinedInAttrTests.swift

-      ]
+      ],
+      fixedSource: #"""
+        @_originallyDefinedIn(modulemodulename: "foo", OSX 13.13)


I would expect modulename was replaced by module here

kimdv · 2023-04-17T09:32:12Z

Tests/SwiftParserTest/translated/ForeachTests.swift

+          // Parse errors
+          for i  in r {
+          }
+          for i in r { sum = sum + i;}


Maybe newline after { and before }

Tests/SwiftParserTest/translated/ForeachTests.swift

kimdv · 2023-04-19T14:14:41Z

@swift-ci please test

ahoppen

Wow, that’s a big PR. And thanks for catching all those incorrect formattings. I think most of the space-related issues should be fixed in the next couple of days by my changes to BasicFormat. All the other ones look like real problems to me. Shows that requiring you to specify fixedSource does provide value 🤩 .

What do you think about the following? We wait for my BasicFormat changes to get in and see which tests are still odd after that. For all of those, you file issues on GitHub (and feel free to mark them as “good first issue” if you think they’ll be easy to fix) and reference those issues as TODOs in the test cases.

kimdv · 2023-04-20T06:08:50Z

Wow, that’s a big PR. And thanks for catching all those incorrect formattings. I think most of the space-related issues should be fixed in the next couple of days by my changes to BasicFormat. All the other ones look like real problems to me. Shows that requiring you to specify fixedSource does provide value 🤩 .

Yes it gives a lot 🙌

What do you think about the following? We wait for my BasicFormat changes to get in and see which tests are still odd after that. For all of those, you file issues on GitHub (and feel free to mark them as “good first issue” if you think they’ll be easy to fix) and reference those issues as TODOs in the test cases.

Let's wait yes, there will 100% be cases that are fixed now.
I will go trough the other files and open issues 😁

kimdv · 2023-04-25T19:43:27Z

@ahoppen it's now rebased on latest main and all swift parser tests passes locally.

ahoppen

Thank you for pushing through all of these test cases. I have added comments to all the tests that would deserve a GitHub issue. Some of the comments can probably be bundled into a single issue. If you could file those, that’d be great.

I also don’t think that we need to add TODOs to the codebase, the issues are sufficient.

Feel free to merge this whenever you get it passing without racing other PRs.

And I don’t think we need to cherry-pick this to release/5.9.

Tests/SwiftParserTest/Assertions.swift

Tests/SwiftParserTest/AttributeTests.swift

Tests/SwiftParserTest/AvailabilityTests.swift

Tests/SwiftParserTest/DeclarationTests.swift

Tests/SwiftParserTest/translated/TypealiasTests.swift

Tests/SwiftParserTest/ExpressionTests.swift

Tests/SwiftParserTest/translated/RecoveryTests.swift

Tests/SwiftParserTest/Assertions.swift

kimdv · 2023-04-29T09:09:14Z

@swift-ci please test

kimdv · 2023-04-29T09:10:08Z

@swift-ci please test windows

kimdv requested a review from ahoppen as a code owner April 14, 2023 22:11

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch 7 times, most recently from 40e3036 to ed090a5 Compare April 17, 2023 09:27

ahoppen reviewed Apr 17, 2023

View reviewed changes

Tests/SwiftParserTest/Assertions.swift Outdated Show resolved Hide resolved

Tests/SwiftParserTest/AttributeTests.swift Outdated Show resolved Hide resolved

kimdv mentioned this pull request Apr 18, 2023

Add diagnostic for wrong inheritance #1450

Merged

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from ed090a5 to d502408 Compare April 18, 2023 20:18

kimdv commented Apr 18, 2023

View reviewed changes

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from d502408 to 7b6b65b Compare April 19, 2023 14:14

ahoppen reviewed Apr 20, 2023

View reviewed changes

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch 2 times, most recently from 819c718 to 1ade024 Compare April 23, 2023 17:45

kimdv requested a review from ahoppen April 23, 2023 17:45

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch 3 times, most recently from 97ee944 to b23d0f5 Compare April 25, 2023 19:42

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch 3 times, most recently from 2d0c998 to 48d25f6 Compare April 26, 2023 18:45

ahoppen approved these changes Apr 28, 2023

View reviewed changes

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from 48d25f6 to b42d76b Compare April 28, 2023 07:06

kimdv mentioned this pull request Apr 28, 2023

FixItApplier is not applying all Fix-Its #1600

Closed

kimdv mentioned this pull request Apr 28, 2023

Braces are not always indented correctly when applying Fix-Its #1601

Open

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from b42d76b to 3d87674 Compare April 28, 2023 12:02

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from 3d87674 to 85be5e8 Compare April 28, 2023 17:24

This was referenced Apr 28, 2023

"insert 'set'" Fix-It is not removing get #1611

Closed

Add newline after } to separate from declarations #1612

Closed

10e10 is not emitted as an invalid version tuple #1613

Closed

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from 85be5e8 to aa84fcd Compare April 28, 2023 18:03

Add assertion for missing fixed source

6261d79

kimdv force-pushed the kimdv/add-assertion-for-missing-fixed-source branch from aa84fcd to 6261d79 Compare April 29, 2023 09:02

kimdv merged commit 46701ae into swiftlang:main Apr 29, 2023

kimdv deleted the kimdv/add-assertion-for-missing-fixed-source branch April 29, 2023 11:22

Add assertion for missing fixed source #1549

Add assertion for missing fixed source #1549

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ahoppen commented Apr 14, 2023

Uh oh!

Uh oh!

Uh oh!

kimdv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kimdv commented Apr 19, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

kimdv commented Apr 20, 2023

Uh oh!

kimdv commented Apr 25, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kimdv commented Apr 29, 2023

Uh oh!

kimdv commented Apr 29, 2023

Uh oh!

Uh oh!

kimdv commented Apr 14, 2023 •

edited

Loading

kimdv commented Apr 25, 2023 •

edited

Loading