Introduce leaf protocols to prevent leaf nodes from being casted #2108

Matejkob · 2023-08-26T20:41:02Z

To-Do after concept approval:

Fix all warnings related to API changes across the entire repo
Add documentation for Code Generation and the new API

I'm opening this PR as a draft to initiate a discussion about the proposed changes.

In issue #2092, @ahoppen mentioned:

What we should do instead, is to only have a as(_: ExprSyntaxProtocol.Type) function on ExprSyntax (analogous for StmtSyntax and the other base nodes).

The as function on SyntaxProtocol can then be marked as deprecated and produce a warning that the cast will always fail.

Best to my understanding, we cannot remove the as method from SyntaxProtocol. This function is necessary due to entities like Syntax or ExprSyntax. Without these entities, we could use the is/as Swift keywords. To illustrate, consider the example below:

func visit(node: some SyntaxProtocol) {
  if node is ClassExprSyntax { ... }
}

The following argument can be passed to this function:

let node = Syntax(ClassExprSyntax("class Foo {}"))

visit(node)

In this scenario, using the is Swift keyword won't correctly identify that the argument is actually of type ClassExprSyntax.

Consequently, I've tried to enhance the downcasting API in a slightly more intricate manner by adding casting methods for lower types.

If any decisions are unclear, please let me know. I'm also eager to hear your general feedback!

This PR resolves part of #2092.

Tests/SwiftRefactorTest/ReformatIntegerLiteral.swift

ahoppen

The approach looks good to me. I have one suggestion of how to avoid all the generated code.

Out of curiosity: How many warnings does this create in swift-syntax?

ahoppen · 2023-08-28T19:32:59Z

Sources/SwiftSyntax/generated/syntaxNodes/SyntaxNodesAB.swift

+
+  @available(*, deprecated, message: "This cast will always fail")
+  public func `is`<S: SyntaxProtocol>(_ syntaxType: S.Type) -> Bool {
+    return self.as(syntaxType) != nil
+  }
+
+  @available(*, deprecated, message: "This cast will always fail")
+  public func `as`<S: SyntaxProtocol>(_ syntaxType: S.Type) -> S? {
+    return S.init(self)
+  }
+
+  @available(*, deprecated, message: "This cast will always fail")
+  public func cast<S: SyntaxProtocol>(_ syntaxType: S.Type) -> S {
+    return self.as(S.self)!
+  }


I think we should be able to avoid generating all these methods by creating the following protocol and then conforming all the non-base nodes to it.

/// Protocol that syntax nodes conform to which don’t have any semantic subtypes, i.e. syntax nodes that aren't base nodes. public protocol LeafSyntaxNodeProtocol: SyntaxProtocol {} public extension LeafSyntaxNodeProtocol { @available(*, deprecated, message: "This cast will always fail") public func `is`<S: SyntaxProtocol>(_ syntaxType: S.Type) -> Bool { return self.as(syntaxType) != nil } @available(*, deprecated, message: "This cast will always fail") public func `as`<S: SyntaxProtocol>(_ syntaxType: S.Type) -> S? { return S.init(self) } @available(*, deprecated, message: "This cast will always fail") public func cast<S: SyntaxProtocol>(_ syntaxType: S.Type) -> S { return self.as(S.self)! } }

Oh yeah, I believe we should be able to do that. Thanks!

This cast will always fail

Isn't necessarily true right 😅? They could be casting to the same type. I suspect that's usually not the case though, so probably makes sense still.

In our codebase, there are a few instances where we're casting leaf nodes to their own types. I've assumed this is redundant and unnecessary. Have I overlooked something? 😅

Oh, I've just realized you probably meant the content of the message, not the mechanism. 😄 I need coffee 🙈

Sorry, to be clear I was referring to deprecation message "This cast will always fail". When we're casting to the same type it won't fail, it's just pointless. The message is fine, I was just being pedantic :)

Introduced 🎸

Good comment, @bnbarham.

I think we should introduce the as/is/cast methods as follows on SyntaxProtocol that will catch same-type conversions an emit a warning the the cast will always succeed, rather than fail.

@available(*, deprecated, message: "This cast will always succeed") func `as`(_ syntaxType: Self.Type) -> Self? { return self }

Really good idea! Implemented in 9986d57

Matejkob · 2023-09-01T17:56:07Z

Out of curiosity: How many warnings does this create in swift-syntax?

With changes to also choice nodes something around 25 warnings. Speaking about choice nodes I'll introduce changes to those nodes in separate PR.

ahoppen

Nice. Looks very good to me. Just a couple comments.

ahoppen · 2023-09-01T18:05:07Z

CodeGeneration/Sources/SyntaxSupport/SyntaxNodeKind.swift

+  /// access it for a non-base kind will result in a runtime error.
+  public var leafProtocolType: TypeSyntax {
+    if isBase {
+      return "Leaf\(syntaxType)NodeProtocol"


Sorry, I should have noted this earlier. Since this protocol needs to be public so that that methods defined within it are visible publicly but since we don’t want to offer any guarantees about supporting it in the future, we should prefix it with an underscore.

Suggested change

return "Leaf\(syntaxType)NodeProtocol"

return "_Leaf\(syntaxType)NodeProtocol"

ahoppen · 2023-09-01T18:08:37Z

Release Notes/510.md

@@ -11,6 +11,11 @@

 ## API Behavior Changes

+- Leaf Node Protocols
+  - Description: Syntax nodes that do not act as base nodes for other syntax types now conform to a new protocols that deprecates inherited casting methods (e.g. `AccessorBlockSyntax` to `LeafSyntaxNodeProtocol` or `AccessorDeclSyntax` to `LeafDeclSyntaxNodeProtocol`). This change aims to prevent unsafe type-casting by issuing deprecation warnings for methods that will result in failed casts.


I would try to avoid the introduction of the protocols since it’s just a technical detail.

Suggested change

- Description: Syntax nodes that do not act as base nodes for other syntax types now conform to a new protocols that deprecates inherited casting methods (e.g. `AccessorBlockSyntax` to `LeafSyntaxNodeProtocol` or `AccessorDeclSyntax` to `LeafDeclSyntaxNodeProtocol`). This change aims to prevent unsafe type-casting by issuing deprecation warnings for methods that will result in failed casts.

- Description: Syntax nodes that do not act as base nodes for other syntax types have the casting methods marked as deprecated. This prevents unsafe type-casting by issuing deprecation warnings for methods that will always result in failed casts.

Thanks, done

ahoppen · 2023-09-01T18:16:54Z

Sources/SwiftSyntax/generated/syntaxNodes/SyntaxNodesAB.swift

+
+  @available(*, deprecated, message: "This cast will always fail")
+  public func `is`<S: SyntaxProtocol>(_ syntaxType: S.Type) -> Bool {
+    return self.as(syntaxType) != nil
+  }
+
+  @available(*, deprecated, message: "This cast will always fail")
+  public func `as`<S: SyntaxProtocol>(_ syntaxType: S.Type) -> S? {
+    return S.init(self)
+  }
+
+  @available(*, deprecated, message: "This cast will always fail")
+  public func cast<S: SyntaxProtocol>(_ syntaxType: S.Type) -> S {
+    return self.as(S.self)!
+  }


Good comment, @bnbarham.

I think we should introduce the as/is/cast methods as follows on SyntaxProtocol that will catch same-type conversions an emit a warning the the cast will always succeed, rather than fail.

@available(*, deprecated, message: "This cast will always succeed") func `as`(_ syntaxType: Self.Type) -> Self? { return self }

ahoppen · 2023-09-01T18:19:45Z

Tests/SwiftSyntaxBuilderTest/BreakStmtSyntaxTests.swift

-      #line: (BreakStmtSyntax().as(StmtSyntax.self)!, "break"),
+      #line: (StmtSyntax(BreakStmtSyntax()), "break"),


Hmm, we should support BreakStmtSyntax().as(StmtSyntax.self). It definitely shouldn’t emit a warning that the cast will always fail because that’s not true, it will always succeed.

I think in LeafDeclSyntaxNodeProtocol, we need the following (same for is and cast).

func `as`(_ syntaxType: DeclSyntax.Type) -> Self? { return self }

I've been thinking more about producing a warning when the user is trying to case to the base node with a message like:

`as` method should be used to downcasting for upcasting to base node use `BaseNode.init`

What do you think?

What about making it slightly more action-focused?

Use `BaseNode.init` for upcasting.

I've added such a feature for all base nodes and Syntax node here: 9986d57

Matejkob · 2023-09-03T20:21:40Z

In a separate PR, I'm going to address casting issues related to syntax traits and syntax choice nodes.

I'm thinking about changing the approach here a bit: Rather than scattering bits of casting logic across multiple files, it might be more efficient to consolidate all the casting functionality for each node and case into a single file. This could make it easier to grasp the overall logic and any edge cases.

How does that sound?

ahoppen

One minor comment on the release notes, otherwise LGTM.

I'm thinking about changing the approach here a bit: Rather than scattering bits of casting logic across multiple files, it might be more efficient to consolidate all the casting functionality for each node and case into a single file. This could make it easier to grasp the overall logic and any edge cases.
How does that sound?

I think that could be a good idea. Let’s do it in a follow-up PR. That way we can get this behavior change merged and the discussion on the follow-up PR can be purely around the implementation structuring.

ahoppen · 2023-09-05T20:16:18Z

Release Notes/510.md

+- Leaf Node Casts
+  - Description: Syntax nodes that do not act as base nodes for other syntax types have the casting methods marked as deprecated. This prevents unsafe type-casting by issuing deprecation warnings for methods that will always result in failed casts.
+  - Issue: https://github.com/apple/swift-syntax/issues/2092
+  - Pull Request: https://github.com/apple/swift-syntax/pull/2108
+
+- Same-Type Casts 
+  - Description: `is`, `as`, and `cast` overloads on `SyntaxProtocol` with same-type conversions are marked as deprecated. The deprecated methods emit a warning indicating the cast will always succeed.
+  - Issue: https://github.com/apple/swift-syntax/issues/2092
+  - Pull Request: https://github.com/apple/swift-syntax/pull/2108
+
+- Base Node Casts
+  - Description: `is`, `as`, and `cast` methods on base node protocols with base-type conversions are marked as deprecated. The deprecated methods emit a warning that informs the developer that the cast will always succeed and should be done using the base node's initializer.
+  - Issue: https://github.com/apple/swift-syntax/issues/2092
+  - Pull Request: https://github.com/apple/swift-syntax/pull/2108
+


I think this should go in the Deprecations sections since the runtime behavior of the API doesn’t change.

ahoppen · 2023-09-05T20:21:54Z

@swift-ci Please test

This commit addresses an issue in the implementation of `SyntaxProtocol` where the 'as' method allows casting to any other syntax node type. This leads to problematic scenarios, such as casting a 'FunctionDeclSyntax' to an 'IdentifierExprSyntax' without compiler warnings, despite the cast being destined to fail at runtime. To solve this, specialized leaf protocols have been introduced. These restrict casting options to only those that are meaningful within the same base node type, thereby enhancing type safety and reducing the risk of runtime errors.

- Implement overloads for `is`, `as`, and `cast` methods in SyntaxProtocol to handle same-type conversions. - Mark these overloaded methods as deprecated with a warning message that the cast will always succeed. - Update the codebase to eliminate a redundant casts.

ahoppen · 2023-09-06T18:05:58Z

@swift-ci Please test

ahoppen · 2023-09-06T18:06:57Z

@swift-ci Please test Windows

Matejkob commented Aug 26, 2023

View reviewed changes

Tests/SwiftRefactorTest/ReformatIntegerLiteral.swift Outdated Show resolved Hide resolved

Matejkob mentioned this pull request Aug 28, 2023

Prevent unrelated casts using as #2092

Open

ahoppen reviewed Aug 28, 2023

View reviewed changes

Matejkob force-pushed the prevent-unreleated-casts branch from b33c407 to b7ce72f Compare September 1, 2023 17:43

Matejkob changed the title ~~[POC] Improve nodes downcasting API~~ Introduce leaf protocols to prevent leaf nodes from being casted Sep 1, 2023

Matejkob marked this pull request as ready for review September 1, 2023 17:56

Matejkob requested review from bnbarham and ahoppen September 1, 2023 17:56

Matejkob force-pushed the prevent-unreleated-casts branch from b7ce72f to 1e8a59c Compare September 1, 2023 18:08

ahoppen reviewed Sep 1, 2023

View reviewed changes

Matejkob force-pushed the prevent-unreleated-casts branch from 1e8a59c to 9986d57 Compare September 3, 2023 20:10

ahoppen reviewed Sep 5, 2023

View reviewed changes

Matejkob force-pushed the prevent-unreleated-casts branch from 9986d57 to b757ea2 Compare September 6, 2023 07:38

Matejkob added 3 commits September 6, 2023 09:43

Add deprecated warnings for base node casts in base node protocols

d2945f8

Matejkob force-pushed the prevent-unreleated-casts branch from b757ea2 to d2945f8 Compare September 6, 2023 07:46

ahoppen approved these changes Sep 6, 2023

View reviewed changes

ahoppen enabled auto-merge September 6, 2023 18:06

ahoppen merged commit a3ef07d into swiftlang:main Sep 6, 2023
3 checks passed

Matejkob mentioned this pull request Sep 6, 2023

Remove unnecessary casting swiftlang/swift-format#618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce leaf protocols to prevent leaf nodes from being casted #2108

Introduce leaf protocols to prevent leaf nodes from being casted #2108

Matejkob commented Aug 26, 2023 •

edited

Loading

ahoppen left a comment

ahoppen Aug 28, 2023

Matejkob Aug 28, 2023

bnbarham Aug 31, 2023

Matejkob Aug 31, 2023 •

edited

Loading

Matejkob Aug 31, 2023

bnbarham Aug 31, 2023 •

edited

Loading

Matejkob Sep 1, 2023

ahoppen Sep 1, 2023

Matejkob Sep 3, 2023

Matejkob commented Sep 1, 2023

ahoppen left a comment

ahoppen Sep 1, 2023

Matejkob Sep 3, 2023

ahoppen Sep 1, 2023

Matejkob Sep 3, 2023

ahoppen Sep 1, 2023

ahoppen Sep 1, 2023

Matejkob Sep 1, 2023

ahoppen Sep 1, 2023 •

edited

Loading

Matejkob Sep 3, 2023

Matejkob commented Sep 3, 2023

ahoppen left a comment

ahoppen Sep 5, 2023

Matejkob Sep 6, 2023

ahoppen commented Sep 5, 2023

ahoppen commented Sep 6, 2023

ahoppen commented Sep 6, 2023

	return "Leaf\(syntaxType)NodeProtocol"
	return "_Leaf\(syntaxType)NodeProtocol"

	- Description: Syntax nodes that do not act as base nodes for other syntax types now conform to a new protocols that deprecates inherited casting methods (e.g. `AccessorBlockSyntax` to `LeafSyntaxNodeProtocol` or `AccessorDeclSyntax` to `LeafDeclSyntaxNodeProtocol`). This change aims to prevent unsafe type-casting by issuing deprecation warnings for methods that will result in failed casts.
	- Description: Syntax nodes that do not act as base nodes for other syntax types have the casting methods marked as deprecated. This prevents unsafe type-casting by issuing deprecation warnings for methods that will always result in failed casts.

		#line: (BreakStmtSyntax().as(StmtSyntax.self)!, "break"),
		#line: (StmtSyntax(BreakStmtSyntax()), "break"),

Introduce leaf protocols to prevent leaf nodes from being casted #2108

Introduce leaf protocols to prevent leaf nodes from being casted #2108

Conversation

Matejkob commented Aug 26, 2023 • edited Loading

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

Matejkob Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham Aug 31, 2023 • edited Loading

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

Matejkob commented Sep 1, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matejkob commented Sep 3, 2023

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

ahoppen commented Sep 5, 2023

ahoppen commented Sep 6, 2023

ahoppen commented Sep 6, 2023

Matejkob commented Aug 26, 2023 •

edited

Loading

Matejkob Aug 31, 2023 •

edited

Loading

bnbarham Aug 31, 2023 •

edited

Loading

ahoppen Sep 1, 2023 •

edited

Loading