[ASTGen] Make ASTGenVisitor not conform to SyntaxTransformVisitor #68574

saehejkang · 2023-09-17T20:36:49Z

Removal of conformance of ASTGenVisitor to SyntaxTransformVisitor.
Implementation of a visit(Syntax) method similar to SytnaxTransformVisitor.visit(Syntax) but that only handles the correct nodes in ASTGen.
Renamed the visit methods to generate.

Resolves #68350

ahoppen

Looks fairly good to me. I just have one suggestion of how you can avoid the Syntax, ie. the places where you currently need to call the Syntax initializer.

A couple of other things that should be part of this PR:

You should be able to remove visitAny in ASTGen.swift:98 now
visit(_: SourceFileSyntax) -> ASTNode in ASTGen.swift:94 also isn’t needed anymore
The uncommented method visit(_: T?) -> [UnsafeMutableRawPointer]? in ASTGen.swift:88 is outdated, not used, and can be removed`
There are still a few visit methods in ASTGen that should be renamed to generate. Searching for func visit shouldn’t give any results in ASTGen anymore.

lib/ASTGen/Sources/ASTGen/ASTGen.swift

lib/ASTGen/Sources/ASTGen/Exprs.swift

saehejkang · 2023-09-18T23:20:28Z

I rebuilt all the test dependencies and ran all the test with this command

utils/run-test --lit ../llvm-project/llvm/utils/lit/lit.py \
  ../build/Ninja-RelWithDebInfoAssert/swift-macosx-$(uname -m)/test-macosx-$(uname -m)

I had an error with the test/ASTGen/verify-parse.swift . I did not see any steps in the docs for adding the test folder to the xcodeproj so I did not make changes to the file.

Any tips when making code changes and having to update/write tests? There are lots of moving pieces and the repo is very big but I want to grasp how testing works and where I would need to make updates anytime I make a change.

ahoppen · 2023-09-19T01:44:52Z

I had an error with the test/ASTGen/verify-parse.swift . I did not see any steps in the docs for adding the test folder to the xcodeproj so I did not make changes to the file.

Any tips when making code changes and having to update/write tests? There are lots of moving pieces and the repo is very big but I want to grasp how testing works and where I would need to make updates anytime I make a change.

What I usually do, is opening the test files in a separate text editor that’s not Xcode. Alternatively, you can also navigate to test/ASTGen/verify-parse.swift in Finder/Terminal and open the file from there. But I wouldn’t expect any changes in the tests to be necessary, really.

lib/ASTGen/Sources/ASTGen/ASTGen.swift

saehejkang · 2023-09-19T02:25:11Z

But I wouldn’t expect any changes in the tests to be necessary, really

When would I know if it is appropriate to updates/add tests? For this issue, do we not need to do any testing because it is just updating function calls/names?

lib/ASTGen/Sources/ASTGen/ASTGen.swift

ahoppen

One formatting comment, otherwise LGTM.

lib/ASTGen/Sources/ASTGen/ASTGen.swift

ahoppen · 2023-09-23T00:59:17Z

@swift-ci Please smoke test

ahoppen · 2023-09-26T00:43:24Z

CI failed with

swiftASTGen/ASTGen.swift:274: Fatal error: not implemented
Please submit a bug report (https://swift.org/contributing/#reporting-bugs) and include the crash backtrace.
Stack dump:
0.  Program arguments: /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swift-macosx-x86_64/bin/swift-frontend -target x86_64-apple-macosx10.13 -module-cache-path /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.13/clang-module-cache -sdk /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -swift-version 4 -define-availability "SwiftStdlib 9999:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999" -define-availability "SwiftStdlib 5.0:macOS 10.14.4, iOS 12.2, watchOS 5.2, tvOS 12.2" -define-availability "SwiftStdlib 5.1:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0" -define-availability "SwiftStdlib 5.2:macOS 10.15.4, iOS 13.4, watchOS 6.2, tvOS 13.4" -define-availability "SwiftStdlib 5.3:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0" -define-availability "SwiftStdlib 5.4:macOS 11.3, iOS 14.5, watchOS 7.4, tvOS 14.5" -define-availability "SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0" -define-availability "SwiftStdlib 5.6:macOS 12.3, iOS 15.4, watchOS 8.5, tvOS 15.4" -define-availability "SwiftStdlib 5.7:macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0" -define-availability "SwiftStdlib 5.8:macOS 13.3, iOS 16.4, watchOS 9.4, tvOS 16.4" -define-availability "SwiftStdlib 5.9:macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0" -define-availability "SwiftStdlib 5.10:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999" -typo-correction-limit 10 /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/ASTGen/verify-parse.swift -dump-parse -disable-availability-checking -enable-experimental-feature ParserASTGen
1.  Apple Swift version 5.11-dev (LLVM 69cd2190ea64502, Swift a23eaeac64bc974)
2.  Compiling with effective version 4.1.50
3.  While evaluating request ParseSourceFileRequest(source_file "/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/ASTGen/verify-parse.swift")
4.  With parser at source location: <invalid loc>
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  swift-frontend           0x000000011469e177 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 39
1  swift-frontend           0x000000011469d1f8 llvm::sys::RunSignalHandlers() + 248
2  swift-frontend           0x000000011469e7d0 SignalHandler(int) + 288
3  libsystem_platform.dylib 0x00007ff805906dfd _sigtramp + 29
4  libsystem_platform.dylib 0x00007ff7b1251780 _sigtramp + 18446744072293231008
5  swift-frontend           0x000000011035fb23 $s11swiftASTGen0B7VisitorV8generateyAA7ASTNodeO11SwiftSyntax0G0VF + 7011
6  swift-frontend           0x000000011035d994 $s11swiftASTGen0B7VisitorV8generateySaySvG11SwiftSyntax010SourceFileF0VF + 836
7  swift-frontend           0x0000000110360969 $s11swiftASTGen21buildTopLevelASTNodes13diagEnginePtr010sourceFileI02dc3ctx13outputContext8callbackySpys5UInt8VG_SPyAJGS3vySv_SvtXCtF + 297
8  swift-frontend           0x00000001102e6dd9 swift::Parser::parseSourceFileViaASTGen(llvm::SmallVectorImpl<swift::ASTNode>&, llvm::Optional<swift::DiagnosticTransaction>&, bool) + 569
9  swift-frontend           0x00000001102e67ba swift::Parser::parseTopLevelItems(llvm::SmallVectorImpl<swift::ASTNode>&) + 42
10 swift-frontend           0x0000000110346545 swift::ParseSourceFileRequest::evaluate(swift::Evaluator&, swift::SourceFile*) const + 389
11 swift-frontend           0x0000000110346f21 swift::SimpleRequest<swift::ParseSourceFileRequest, swift::SourceFileParsingResult (swift::SourceFile*), (swift::RequestFlags)12>::evaluateRequest(swift::ParseSourceFileRequest const&, swift::Evaluator&) + 17
12 swift-frontend           0x0000000110648b7e llvm::Expected<swift::ParseSourceFileRequest::OutputType> swift::Evaluator::getResultUncached<swift::ParseSourceFileRequest>(swift::ParseSourceFileRequest const&) + 414
13 swift-frontend           0x00000001106488bc llvm::Expected<swift::ParseSourceFileRequest::OutputType> swift::Evaluator::getResultCached<swift::ParseSourceFileRequest, (void*)0>(swift::ParseSourceFileRequest const&) + 156
14 swift-frontend           0x00000001106257d5 swift::ParseSourceFileRequest::OutputType swift::evaluateOrDefault<swift::ParseSourceFileRequest>(swift::Evaluator&, swift::ParseSourceFileRequest, swift::ParseSourceFileRequest::OutputType) + 37
15 swift-frontend           0x000000011061b40b swift::SourceFile::getTopLevelItems() const + 123
16 swift-frontend           0x00000001103f1f06 swift::SourceFile::dump(llvm::raw_ostream&, bool) const + 38
17 swift-frontend           0x000000010ef02fe3 dumpAST(swift::CompilerInstance&) + 467
18 swift-frontend           0x000000010eef5006 performCompile(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 2038
19 swift-frontend           0x000000010eef391b swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 3243
20 swift-frontend           0x000000010ece354a swift::mainEntry(int, char const**) + 2282
21 dyld                     0x000000012bf7e52e start + 462
/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/ASTGen/Output/verify-parse.swift.script: line 6: 55123 Illegal instruction: 4  /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swift-macosx-x86_64/bin/swift-frontend -target x86_64-apple-macosx10.13 -module-cache-path /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.13/clang-module-cache -sdk '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk' -swift-version 4 -define-availability 'SwiftStdlib 9999:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999' -define-availability 'SwiftStdlib 5.0:macOS 10.14.4, iOS 12.2, watchOS 5.2, tvOS 12.2' -define-availability 'SwiftStdlib 5.1:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0' -define-availability 'SwiftStdlib 5.2:macOS 10.15.4, iOS 13.4, watchOS 6.2, tvOS 13.4' -define-availability 'SwiftStdlib 5.3:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0' -define-availability 'SwiftStdlib 5.4:macOS 11.3, iOS 14.5, watchOS 7.4, tvOS 14.5' -define-availability 'SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0' -define-availability 'SwiftStdlib 5.6:macOS 12.3, iOS 15.4, watchOS 8.5, tvOS 15.4' -define-availability 'SwiftStdlib 5.7:macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0' -define-availability 'SwiftStdlib 5.8:macOS 13.3, iOS 16.4, watchOS 9.4, tvOS 16.4' -define-availability 'SwiftStdlib 5.9:macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0' -define-availability 'SwiftStdlib 5.10:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999' -typo-correction-limit 10 /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/ASTGen/verify-parse.swift -dump-parse -disable-availability-checking -enable-experimental-feature ParserASTGen > /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/ASTGen/Output/verify-parse.swift.tmp/astgen.ast

Could you debug the failure?

saehejkang · 2023-09-26T01:26:43Z

Could you debug the failure?

This is a weird issue because all I did was add new lines between the functions and it broke the CI. I just pushed up changes to resolve merge conflicts and updated the branch completely. Maybe that is the fix?

etcwilde · 2023-09-26T04:40:41Z

This is a weird issue because all I did was add new lines between the functions and it broke the CI. I just pushed up changes to resolve merge conflicts and updated the branch completely. Maybe that is the fix?

Unlikely. You've made quite a few changes, including entirely rewriting the function that contains the tripping fatal error. According to the stacktrace, you're fatal erroring in swiftASTGen.ASTGenVisitor.generate(SwiftSyntax.Syntax) -> swiftASTGen.ASTNode: https://github.com/apple/swift/pull/68574/files#diff-627bedb4ff4c21e0c31e3af2850fdc80643131077504c697cd31e4a4af16799dR274

You'll need to figure out what cases you're missing and either figure out why you're hitting them if they're not supposed to be hit, or implement them.

saehejkang · 2023-09-27T00:31:22Z

@ahoppen could you run the tests please?

lib/ASTGen/Sources/ASTGen/ASTGen.swift

etcwilde · 2023-09-28T00:00:01Z

@swift-ci please test

ahoppen · 2023-09-28T15:54:05Z

@swift-ci Please test Linux

hamishknight · 2023-09-29T09:32:30Z

@swift-ci please test Linux

ahoppen · 2023-09-29T12:39:27Z

@swift-ci Please test Linux

AnthonyLatsis

Sorry to chime in so late!

lib/ASTGen/Sources/ASTGen/ASTGen.swift

saehejkang · 2023-09-30T18:48:44Z

recent changes pushed to main are not 100% working. On line 161, I am getting the error

Value of type 'TypeEffectSpecifiersSyntax' has no member 'thrownError'

This same issue is ocurring on the main branch

AnthonyLatsis · 2023-10-01T02:59:10Z

recent changes pushed to main are not 100% working

swift/main depends on swift-syntax/main when the early swift-syntax build is enabled. Did you manually pull changes? Normally, you should rely on update-checkout instead to sync up with dependencies.

AnthonyLatsis

Just a couple more remarks. Once resolved, please clean up the commit history, and we’ll be ready to go for merge.

lib/ASTGen/Sources/ASTGen/ASTGen.swift

AnthonyLatsis · 2023-10-01T19:22:13Z

How'd it go? Need help?

saehejkang · 2023-10-01T19:28:08Z

How'd it go? Need help?

It went alright, I think. I cleaned up past commits but I don't think it was 100% right. After I ran git rebase main, I ran the git rebase -i HEAD~N and changed all commits to squash except the first one.

[ASTGen]: Implementation of visit(syntax) method

I then force pushed to the PR and it did remove all the extra commits I wanted gone. But then, it added all past commits from others (which is why it added them as reviewers), and I simply did the same thing but changed commits to drop. However, it was outdated with main and I had to force push main again. There are now 3 commits instead of 1.

bnbarham · 2023-10-02T18:39:20Z

It went alright, I think. I cleaned up past commits but I don't think it was 100% right. After I ran git rebase main, I ran the git rebase -i HEAD~N and changed all commits to squash except the first one.

You can just run git rebase -i main in this case, which maybe we should change the docs to suggest over HEAD~N. The second commit can be squashed and the last commit shouldn't be needed at all.

But then, it added all past commits from others (which is why it added them as reviewers), and I simply did the same thing but changed commits to drop. However, it was outdated with main and I had to force push main again. There are now 3 commits instead of 1.

It's not clear to me how this could have happened, did you do any re-ordering of commits in the rebase?

saehejkang · 2023-10-03T03:13:50Z

did you do any re-ordering of commits in the rebase

I did not do any reordering of commits in the rebase. Should I cleanup the 3 commits or is what I have fine?

bnbarham · 2023-10-03T04:34:12Z

I did not do any reordering of commits in the rebase. Should I cleanup the 3 commits or is what I have fine?

Squashing them would be appreciated 🙇‍♂️

AnthonyLatsis · 2023-10-04T00:01:57Z

Should I cleanup the 3 commits or is what I have fine?

After squashing and getting rid of the merge commit, please consider breaking up your changes into at least 2 meaningful commits rather than cramming everything into a single one. For example, the first one removes the conformance, and the second renames the methods.

bnbarham · 2023-10-04T02:29:45Z

please consider breaking up your changes into at least 2 meaningful commits rather than cramming everything into a single one

I think it's fine in this case, it's fairly self contained as is.

bnbarham · 2023-10-04T02:29:52Z

@swift-ci please test

bnbarham · 2023-10-04T03:06:53Z

@saehejkang
[ASTGen]: Updates to BridgedSourceLoc function calls

That should definitely be a separate PR, is there any reason you added it to this one?

saehejkang · 2023-10-04T03:07:03Z

@bnbarham I just pushed up one last commit. It should be good to go now.

bnbarham · 2023-10-04T03:08:00Z

@bnbarham I just pushed up one last commit. It should be good to go now.

I saw, see my above comment. I'd prefer that one in a new PR.

saehejkang · 2023-10-04T03:09:05Z

That should definitely be a separate PR, is there any reason you added it to this one?

I added this commit to keep things in line with main. It was out of date by one commit and I think I missed it while rebasing. The changes I have in this PR are reflected correctly and no changes are technichally being made that are different to main.

saehejkang · 2023-10-04T03:16:49Z

Ok @bnbarham I got the rebase working correctly and now there is only one commit 😎

AnthonyLatsis · 2023-10-04T03:22:09Z

I think it's fine in this case, it's fairly self contained as is.

I don’t mind as much, but would love a more descriptive commit message, like the title of the issue.

saehejkang · 2023-10-04T03:23:11Z

I don’t mind as much, but would love a more descriptive commit message, like the title of the issue.

I don't mind updating it

bnbarham · 2023-10-04T03:24:57Z

I don’t mind as much, but would love a more descriptive commit message, like the title of the issue.

Sure, all for a more descriptive title and description in the commit :)

AnthonyLatsis · 2023-10-04T03:49:37Z

@swift-ci please test

hamishknight

Thanks!

saehejkang requested review from zoecarver and CodaFi as code owners September 17, 2023 20:36

saehejkang marked this pull request as draft September 17, 2023 20:37

ahoppen reviewed Sep 18, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

lib/ASTGen/Sources/ASTGen/Exprs.swift Outdated Show resolved Hide resolved

saehejkang commented Sep 19, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

saehejkang commented Sep 19, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

ahoppen reviewed Sep 19, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

saehejkang marked this pull request as ready for review September 19, 2023 22:33

saehejkang requested a review from ahoppen September 19, 2023 22:34

ahoppen reviewed Sep 20, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

saehejkang requested a review from ahoppen September 22, 2023 03:53

ahoppen approved these changes Sep 23, 2023

View reviewed changes

ahoppen reviewed Sep 27, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

AnthonyLatsis reviewed Sep 29, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

saehejkang requested a review from AnthonyLatsis September 30, 2023 17:52

AnthonyLatsis reviewed Oct 1, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Show resolved Hide resolved

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

AnthonyLatsis removed request for LucianoPAlmeida, hborla and tshortli October 1, 2023 19:21

saehejkang force-pushed the astgen-reconformance branch from 5641ae2 to b28bad5 Compare October 4, 2023 01:59

saehejkang requested review from compnerd and hamishknight as code owners October 4, 2023 01:59

saehejkang force-pushed the astgen-reconformance branch from b28bad5 to d836a23 Compare October 4, 2023 02:00

saehejkang force-pushed the astgen-reconformance branch from 1584f32 to 22fe6d9 Compare October 4, 2023 03:13

[ASTGen]: Make ASTGenVisitor not conform to SyntaxTransformVisitor

a4da276

saehejkang force-pushed the astgen-reconformance branch from 22fe6d9 to a4da276 Compare October 4, 2023 03:32

hamishknight approved these changes Oct 4, 2023

View reviewed changes

hamishknight merged commit ad10c72 into swiftlang:main Oct 4, 2023

saehejkang deleted the astgen-reconformance branch May 23, 2024 00:21

[ASTGen] Make ASTGenVisitor not conform to SyntaxTransformVisitor #68574

[ASTGen] Make ASTGenVisitor not conform to SyntaxTransformVisitor #68574

Uh oh!

Conversation

saehejkang commented Sep 17, 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!

saehejkang commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Sep 19, 2023

Uh oh!

Uh oh!

Uh oh!

saehejkang commented Sep 19, 2023

Uh oh!

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Sep 23, 2023

Uh oh!

ahoppen commented Sep 26, 2023

Uh oh!

saehejkang commented Sep 26, 2023

Uh oh!

etcwilde commented Sep 26, 2023

Uh oh!

saehejkang commented Sep 27, 2023

Uh oh!

Uh oh!

etcwilde commented Sep 28, 2023

Uh oh!

ahoppen commented Sep 28, 2023

Uh oh!

hamishknight commented Sep 29, 2023

Uh oh!

ahoppen commented Sep 29, 2023

Uh oh!

AnthonyLatsis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saehejkang commented Sep 30, 2023

Uh oh!

AnthonyLatsis commented Oct 1, 2023

Uh oh!

AnthonyLatsis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AnthonyLatsis commented Oct 1, 2023

Uh oh!

saehejkang commented Oct 1, 2023

Uh oh!

bnbarham commented Oct 2, 2023

Uh oh!

saehejkang commented Oct 3, 2023

Uh oh!

bnbarham commented Oct 3, 2023

Uh oh!

AnthonyLatsis commented Oct 4, 2023

Uh oh!

bnbarham commented Oct 4, 2023

Uh oh!

bnbarham commented Oct 4, 2023

Uh oh!

bnbarham commented Oct 4, 2023

Uh oh!

saehejkang commented Sep 17, 2023 •

edited

Loading

saehejkang commented Sep 18, 2023 •

edited

Loading

AnthonyLatsis commented Oct 4, 2023 •

edited

Loading