[NFC-ish] Rework compatibility layer generation #2888

beccadax · 2024-10-31T02:57:26Z

This PR replaces the deprecatedName property on Child with a new, more sophisticated mechanism which supports:

• Multiple past changes (for instance, a property that has been renamed more than once) with a different initializer for each past state.
• The ability to represent the order of multiple changes and group together simultaneous changes.
• Multiple kinds of changes, including an extracted change where a group of adjacent nodes are moved into a newly-created child node.

This new childHistory mechanism also works on Traits, where it generates compatibility properties as one would expect. (We previously silently ignored deprecatedName on a child of a trait.)

This PR does not add or remove any SwiftSyntax APIs, but it does cause small, functionally equivalent changes to the generated code.

Extracted from #2882.

ahoppen

Looks like a great clean-up to the somewhat ad-hoc deprecation design we had before.

ahoppen · 2024-10-31T16:40:07Z

CodeGeneration/Sources/SyntaxSupport/Child.swift

@@ -40,7 +40,8 @@ public enum ChildKind {
  /// The child always contains a node of the given `kind`.
  case node(kind: SyntaxNodeKind)
  /// The child always contains a node that matches one of the `choices`.
-  case nodeChoices(choices: [Child])
+  case nodeChoices(choices: [Child], childHistory: Child.History = [])
+  // FIXME: We don't appear to have ever generated compatibility layers for children of node choices!


I think you resolved this comment, right?

nodeChoices still doesn't have compatibility layers. I think largely because Swift can't alias enum cases—you can use a deprecated static method to construct the replacement case, but not to pattern-match it.

Instead of having a FIXME for it (which is very easy to miss), could we raise a fatalError if you try to add a compatibility layer of node choices? That way you can’t miss it if you try to add one.

So, there actually are existing cases where people have specified a deprecatedName: for a child of a ChildKind.nodeChoices; this information was simply ignored until now. Rather than deleting it, I thought it'd be better to preserve the info in the node definition files, but if we'd rather just delete it (which—again—would be an NFC change), I can remove the childHistory value here entirely.

I haven't done anything with this yet, but I think I've addressed everything else you suggested.

Thanks for checking. Could you file an issue for the FIXME? Then we can take a look at it after this PR is merged.

CodeGeneration/Sources/SyntaxSupport/Child.swift

CodeGeneration/Sources/SyntaxSupport/CompatibilityLayers.swift

CodeGeneration/Sources/SyntaxSupport/Child.swift

CodeGeneration/Sources/SyntaxSupport/CompatibilityLayers.swift

CodeGeneration/Sources/generate-swift-syntax/InitSignature+Extensions.swift

...n/Sources/generate-swift-syntax/templates/swiftsyntax/RenamedChildrenCompatibilityFile.swift

ahoppen

Thanks for addressing my comments, @beccadax.

ahoppen · 2024-11-04T16:58:38Z

CodeGeneration/Sources/SyntaxSupport/CompatibilityLayer.swift

+  public var deprecatedMembersByTrait: [String: DeprecatedMemberInfo] = [:]
+
+  /// Cache for `replacementChild(for:)`. Ensures that we don't create
+  /// two different replacement nodes even if we refactor the same child twice.


I would just add

Suggested change

/// two different replacement nodes even if we refactor the same child twice.

/// two different replacement nodes even if we refactor the same child twice.

/// This is important because we use pointer equality of `Child` to decide if two children are the same.

ahoppen · 2024-11-04T16:59:28Z

CodeGeneration/Sources/SyntaxSupport/CompatibilityLayer.swift

+  }
+
+  internal init(nodes: [Node], traits: [Trait]) {
+    // Note that we compute these up-front, greedily, so that all of the mutation is isolated by the lazy var's state.


Which lazy var? Is this a leftover from a previous version?

I was referring to the SYNTAX_COMPATIBILITY_LAYER global, which like all Swift globals, is implicitly lazily initialized with synchronization.

Would this be a better way to describe the situation?

Suggested change

// Note that we compute these up-front, greedily, so that all of the mutation is isolated by the lazy var's state.

// This instance will be stored in a global that's used from multiple threads simultaneously, so it won't be safe

// to mutate once the initializer returns. We therefore do all the work to populate its tables up front, rather

// than computing it lazily on demand.

Sounds great to me. Thanks!

beccadax · 2024-11-04T20:48:54Z

@swift-ci please test

beccadax · 2024-11-04T23:05:07Z

SyntaxSupport/Child.swift:345: Fatal error: unexpected node has no siblings?

Turned out this was happening with base nodes, since they don't have any children (but their unexpected nodes are ignored anyway). Pulled in a fix that avoids trying to create unexpected children for base nodes.

beccadax · 2024-11-04T23:05:28Z

@swift-ci please test

Introduce and factor out a type that represents a list of children for which an initializer ought to exist. This changes some `@available(renamed:)` values, but nothing that should change diagnostics or behavior.

A new CompatibilityLayer type computes information about deprecated vars and inits, presenting it in a new way: by creating additional `Child` objects with the deprecated information, plus sufficient info to map it back to the current name. This causes minor non-functional changes to the bodies of compatibility builder inits, but otherwise has no real effect in this commit. However, this new setup with separate `Child` nodes for deprecated children and a global table of information about them prepares us for further changes.

Introduce a much more sophisticated way of describing past revisions to a node’s children. This allows: • Multiple past changes (for instance, a property that has been renamed more than once). • The ability to represent the order of multiple changes and group together simultaneous changes. • Multiple kinds of changes, including changes that introduce more than one member. Note that this commit does not use any of these new capabilities; it just ports existing behavior to the new representation.

It’s always been possible to specify deprecated children, but nothing was actually done with the info until now. Turns some manaully-generated decls into automatically-generated ones, but doesn’t change anything user-facing.

beccadax · 2024-11-05T02:28:40Z

Needed to placate swift-format.

beccadax · 2024-11-05T02:28:50Z

@swift-ci please smoke test

beccadax · 2024-11-05T03:05:04Z

@swift-ci please test

beccadax requested review from ahoppen and bnbarham as code owners October 31, 2024 02:57

beccadax mentioned this pull request Oct 31, 2024

SwiftSyntax support for experimental @abi attribute #2882

Open

ahoppen reviewed Oct 31, 2024

View reviewed changes

beccadax force-pushed the history-is-childs-play branch from a461ff2 to 75f6c3f Compare November 2, 2024 02:50

ahoppen approved these changes Nov 4, 2024

View reviewed changes

beccadax force-pushed the history-is-childs-play branch from 75f6c3f to e458bdf Compare November 4, 2024 19:23

beccadax force-pushed the history-is-childs-play branch from e458bdf to e5bde56 Compare November 4, 2024 23:02

beccadax added 4 commits November 4, 2024 18:03

[NFC-ish] Factor out InitSignature

4567192

Introduce and factor out a type that represents a list of children for which an initializer ought to exist. This changes some `@available(renamed:)` values, but nothing that should change diagnostics or behavior.

[NFC-ish] Support child history for traits

f1fcb10

It’s always been possible to specify deprecated children, but nothing was actually done with the info until now. Turns some manaully-generated decls into automatically-generated ones, but doesn’t change anything user-facing.

beccadax force-pushed the history-is-childs-play branch from e5bde56 to f1fcb10 Compare November 5, 2024 02:28

beccadax enabled auto-merge November 5, 2024 02:30

beccadax merged commit 0386d87 into swiftlang:main Nov 5, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC-ish] Rework compatibility layer generation #2888

[NFC-ish] Rework compatibility layer generation #2888

beccadax commented Oct 31, 2024 •

edited

Loading

ahoppen left a comment

ahoppen Oct 31, 2024

beccadax Oct 31, 2024 •

edited

Loading

ahoppen Oct 31, 2024

beccadax Oct 31, 2024

beccadax Nov 2, 2024

ahoppen Nov 4, 2024

ahoppen left a comment

ahoppen Nov 4, 2024

ahoppen Nov 4, 2024

beccadax Nov 4, 2024

beccadax Nov 4, 2024

ahoppen Nov 4, 2024

beccadax commented Nov 4, 2024

beccadax commented Nov 4, 2024

beccadax commented Nov 4, 2024

beccadax commented Nov 5, 2024

beccadax commented Nov 5, 2024

beccadax commented Nov 5, 2024

	/// two different replacement nodes even if we refactor the same child twice.
	/// two different replacement nodes even if we refactor the same child twice.
	/// This is important because we use pointer equality of `Child` to decide if two children are the same.

-    // Note that we compute these up-front, greedily, so that all of the mutation is isolated by the lazy var's state.
+    // This instance will be stored in a global that's used from multiple threads simultaneously, so it won't be safe
+    // to mutate once the initializer returns. We therefore do all the work to populate its tables up front, rather
+    // than computing it lazily on demand.

[NFC-ish] Rework compatibility layer generation #2888

[NFC-ish] Rework compatibility layer generation #2888

Conversation

beccadax commented Oct 31, 2024 • edited Loading

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beccadax Oct 31, 2024 • 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

Choose a reason for hiding this comment

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

beccadax commented Nov 4, 2024

beccadax commented Nov 4, 2024

beccadax commented Nov 4, 2024

beccadax commented Nov 5, 2024

beccadax commented Nov 5, 2024

beccadax commented Nov 5, 2024

beccadax commented Oct 31, 2024 •

edited

Loading

beccadax Oct 31, 2024 •

edited

Loading