Cherry-pick some post-4.2 changes into swift-4.2-branch #6
Conversation
* [SwiftSyntax] Add accessors for source locations and test diagnostic emission * Add tests for endLocation * Pre-emptively copy AbsolutePosition to avoid mutating it twice
|
|
||
| /// An absolute position in a source file as text - the absolute utf8Offset from | ||
| /// the start, line, and column. | ||
| public final class AbsolutePosition { |
Sources/SwiftSyntax/RawSyntax.swift
Outdated
| @@ -251,6 +251,13 @@ extension RawSyntax { | |||
| } | |||
| } | |||
|
|
|||
| func accumulateTrailingTrivia(_ pos: AbsolutePosition) { | |||
There was a problem hiding this comment.
This has since been removed on master in favour of trailingTriviaLength so I don't think we should add it back here. Should be part of fc313fc
| XCTAssertEqual(0, parsed.position.utf8Offset) | ||
| XCTAssertEqual(source.count, | ||
| parsed.eofToken.positionAfterSkippingLeadingTrivia.utf8Offset) | ||
| XCTAssertEqual(0, parsed.position.utf8Offset) |
There was a problem hiding this comment.
Can we write the above two XCTAssertEqual to 0 the other way round, i.e. 0 as the second parameter?
| XCTAssertEqual(parsed.byteSize, source.count) | ||
| XCTAssertEqual(source.count, | ||
| parsed.eofToken.positionAfterSkippingLeadingTrivia.utf8Offset) | ||
| XCTAssertEqual(0, parsed.position.utf8Offset) |
There was a problem hiding this comment.
Same here
XCTAssertEqual(parsed.position.utf8Offset, 0)
| XCTAssertEqual(state.leadingTrivia!.byteSize + state.trailingTrivia!.byteSize | ||
| + state.byteSizeAfterTrimmingTrivia, state.byteSize) | ||
| XCTAssertEqual(3, state.leadingTrivia!.count) | ||
| XCTAssertEqual(1, state.trailingTrivia!.count) |
There was a problem hiding this comment.
Again, expected value as the second parameter.
Since absolute position is defined by accumulating proceeding nodes, we should allow its access for nodes without SourceFileSyntax as root.
SourcePresence and ID are already shared between the two node kinds and the node's length will soon be cached in RawSyntax as well. By making it a struct, we will be able to compute the node's length when it is being constructed in the initialiser.
This was originally "[swiftSyntax] Add test cases for the SyntaxClassifier" (apple/swift-syntax e7c90be). We don't actually pull in any SyntaxClassifier logic, but this commit does fix a bug where compiler errors cause the entire parse to fail.
This way we will be able to avoid reclassifying these nodes for syntax highlighting since we know they haven't changed.
AbsolutePosition being a mutable reference type easily leads to bugs where an AbsolutePosition is modified. Making it immutable eliminates this issue. Furthermore, the introduction of SourceLength should allow easier manipulation of AbsolutePositions on the client side. We still cannot make AbsolutePosition a value type since it is used inside AtomicCache, but the immutability gives the same safety.
AbsolutePosition had value semantics anyway, the only reason it was a reference type was so that we can use it in AtomicCache. But that can be worked around by boxing it into a reference type.
The methods were never executed because DEBUG was never defined in normal builds and the only way to create nodes is through generated factory methods which provide the same safety `validate` was supposed to ensure at the interface level.
This change was made post-4.2 in apple/swift's GYB support files. We copy it here because it's generally useful.
62d9c34 to
5d9650b
Compare
|
Good call on the extra cherrypicks. I ended up pulling everything I was able to from The final commit is a workaround for the fact that I can't just cherrypick the change I want for list nodes because the change occurred in apple/swift. All tests still pass locally. Please take a look. |
There was a problem hiding this comment.
I guess cherry-picking almost everything is indeed the cleanest solution. I just want to quickly hear @nkcsgexi 's opinion on it, but should be fine.
Changes look good overall. I've got some minor comments.
Other than that, I'd like to wait until PR testing is set up for this repository before merging. Shouldn't take much longer though.
| let result = try swiftcRunner.invoke() | ||
| return result.stdoutData | ||
| public var description: String { | ||
| switch self{ |
There was a problem hiding this comment.
Space after self. If this is also badly formatted on master, it's my fault but we could still make it clean here.
There was a problem hiding this comment.
Done. (Any style errors in these commits are also in master, since I did straight cherrypicks without modification except in rare cases, but I'm happy to fix them up here.)
| # live in apple/swift, so we've simply copied the updated implementation | ||
| # here and updated the calls below. | ||
| # See: https://github.com/apple/swift/commit/a66931e7 | ||
| def PATCHED_is_visitable(node): |
There was a problem hiding this comment.
I would say that the fact that is_visitable was only introduced post 4.2 is an implementation detail and would thus still name the function is_visitable. IMHO there is no need to prefix it with PATCHED.
There was a problem hiding this comment.
is_visitable was present in/before 4.2, but its implementation has changed; that's why I gave it a different name, to make sure that it didn't collide with the same symbol from gyb_syntax_support/__init__.py and to make it stand out to future readers.
| } | ||
|
|
||
| public class DiagnosticsTestCase: XCTestCase { | ||
| public class DiagnosticTestCase: XCTestCase { |
There was a problem hiding this comment.
Sorry for spelling that test case wrong when I cherry-picked. Could you also rename the file? Also, we should change this on master.
There was a problem hiding this comment.
Rename it to DiagnosticTestCase.swift?
Right now it looks like all of the test files and the test classes inside them are named inconsistently; it might be cleaner to do a separate PR that unifies them all (and do it on both branches) instead of trying to add it to this cherrypick.
There was a problem hiding this comment.
That makes sense. Let's tackle it in another PR then.
| let parsed = try SyntaxTreeParser.parse(getInput("nested-blocks.swift")) | ||
| let visitor = VisitCollections() | ||
| visitor.visit(parsed) | ||
| XCTAssertEqual(4, visitor.numberOfCodeBlockItems) |
There was a problem hiding this comment.
One more occurrence where I'd prefer to have the expected value on the right.
There was a problem hiding this comment.
You and @harlanhaskins have different points of view about expected/actual argument order 🙂 (I don't have a preference either way, so I'll change it since I've already changed the others.)
There was a problem hiding this comment.
Oh, I'm with @ahoppen on this. The only reason I had them as expected-first is because that's the order StdlibUnittest expected them, which made me sad.
# This is the 1st commit message: fixed testAvailabilityQuery34 and testAvailabilityQueryUnavailability28 # This is the commit message swiftlang#2: Update Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift Co-authored-by: Kim de Vos <kimdevos12@hotmail.com> # This is the commit message swiftlang#3: added fixedSource in test case # This is the commit message swiftlang#4: minor changes # This is the commit message swiftlang#5: implemented recovery inside the parser # This is the commit message swiftlang#6: runned format.py # This is the commit message swiftlang#7: minor changes # This is the commit message swiftlang#8: minor changes
Fix an index-out-of-bounds error in DontRepeatTypeInStaticProperties.
These source-location-related APIs are generally useful and safe to cherry-pick into
swift-4.2-branchbecause they do not depend on any syntax schema changes that have occurred since the branch cut. (They are also required by the formatter, and cannot be easily added as extensions in our code base because they need access to private properties.)The final commit is a manual graft of tests from the original set, which couldn't be applied as a simple patch because the tests had been rewritten to use XCTest instead of StdlibUnittest.
This PR was tested by rebasing the changes on top of
swift-4.2-DEVELOPMENT-SNAPSHOT-2018-08-25-aand runningbuild-script.py -r -t. All tests pass.cc @harlanhaskins