[Swift] Add first piece of rule reduction #2692

stephen-hawley · 2024-09-25T16:17:55Z

This PR adds demangling tree reduction to the project.
What does this mean?
When a symbol is demangled there is an intermediate state where there is a tree of Node which represent the semantic meaning of the symbol. In order for this to be useful to us, we need to reduce a tree of Node to a more directly accessible set of types.

To do this, there are 3 main pieces:

a way to determine if a Node (and possibly subnodes) match a given pattern. This is MatchRule
a way to uniformly apply rules (RulesRunner)
a way, given a matched node, to reduce it to a more easily consumable data structure

This PR partially addresses issue 2664.

What happens: given a Swift5Demangler instance, calling Run with a symbol returns an IReduction that represents the result of the reduction. Right now, we're only ever instantiating either a ProtocolWitnessTableReduction, TypeSpec or a ReductionError, but I created placeholder for functions which will help enable dispatch thunks later.

Inner types will not work yet - issue opened here.

The reduction rules are non-static and the collection gets rebuilt for every symbol. This is known.

There is a name argument in the rule which is not used (yet). I'm trying to decide still if we're going to need it.

MatchRule is very flexible - it can do the simplest matching on a single Node.Kind but it can also match on multiples as well. It can match on child nodes too. Overall, this is a simplified version of a regular expression in a way. Nothing in the current code requires it, but we will need it for more complicated reductions later (thunks, for example).

Where this is going: we should be able to get all the protocol witness table symbols from a file with a single expression:

var protoWitnesses = machoFiies.AllPublicSymbols().Select(nle => new Swift5Demangler(nle.str).Run()).OfType(ProtocolWitnessTableReduction);

Similar expressions could be used to get all the dispatch thunks etc.

matouskozak · 2024-09-26T08:10:08Z

src/Swift.Bindings/tests/DemanglerTests/BasicDemanglingTests.cs

+ var symbol = "_$ThisIsJustGarbage";
+ var demangler = new Swift5Demangler (symbol);
+ var result = demangler.Run ();
+ var err = result as ReductionError;


What is expected to happen if we would instead do result as ProtocolWitnessTableReduction? I

I would think that in non-test code you would always switch on the type or check for a null result after the cast. result as ProtocolWitnessTableReduction should return null if result is of the ReductionError type

It would absolutely return null. The test is there to ensure that a symbol that is obviously not a Swift mangled symbol will correctly generate an error. Anything other cast will fail.

matouskozak

Thanks a lot for the great descriptions, it helps a lot in understanding the PR.

Good point on future work to make the BuildMatchRules static #2691 (maybe we could include it as part of this PR already but unsure if there are some hidden obstacles that would make it too complex)

matouskozak · 2024-09-26T08:30:38Z

src/Swift.Bindings/src/Demangler/MatchRule.cs

+/// </summary>
+internal class MatchRule {
+ /// <summary>
+ /// The name of this rule - usefule for debugging


Suggested change

/// The name of this rule - usefule for debugging

/// The name of this rule - useful for debugging

matouskozak · 2024-09-26T08:36:08Z

src/Swift.Bindings/src/Demangler/MatchRule.cs

+ /// <exception cref="InvalidOperationException"></exception>
+ bool ContentMatches(Node n)
+ {
+ // Only care about the content type not its value


Shouldn't we rename it to ContentTypeMatches if we only care about the content type?

matouskozak · 2024-09-26T08:41:24Z

src/Swift.Bindings/src/Demangler/MatchRule.cs

+/// - One or more NodeKinds to match
+/// If a match occurs, a reducer function can be run on the node.
+/// </summary>
+internal class MatchRule {


Do you think it would be beneficial to ask someone from the Roslyn team to review this (and maybe the previously introduced Parser)?

I think this is an area commonly found in the frontend of compilers and they may have some interesting insights for us if we plan to grow this further.

mcumming · 2024-09-26T12:53:46Z

src/Swift.Bindings/src/Demangler/MatchRule.cs

+ /// Creates a simple string representation of this rule
+ /// </summary>
+ /// <returns></returns>
+ public override string ToString() => Name;


Wouold it help to have a DebuggerDisplayAttribute that surfaces this value?

mcumming · 2024-09-26T12:55:36Z

src/Swift.Bindings/src/Demangler/Swift5Reducer.cs

+ /// </summary>
+ string ExpectedButGot (string expected, string butGot)
+ {
+ return $"Demangling {mangledName}: expected {expected} but got {butGot}";


Where do these strings surface? Should they be localized?

Eventually, they should be localized. I'm not concerned with that right now. However, putting the very common "Expected but got" pattern in one place makes that easier down the road.

mcumming · 2024-09-26T12:57:22Z

src/Swift.Bindings/src/Model/SwiftFunction.cs

+ /// Returns a string representation of the function
+ /// </summary>
+ /// <returns></returns>
+ public override string ToString () => $"{Provenance}.{Name}{ParameterList} -> {Return}";


Again, any usefulness in having a DebuggerDisplayAttribute?

mcumming · 2024-09-26T13:00:29Z

src/Swift.Bindings/tests/DemanglerTests/BasicDemanglingTests.cs

+ var symbol = "_$ThisIsJustGarbage";
+ var demangler = new Swift5Demangler (symbol);
+ var result = demangler.Run ();
+ var err = result as ReductionError;


I would think that in non-test code you would always switch on the type or check for a null result after the cast. result as ProtocolWitnessTableReduction should return null if result is of the ReductionError type

kotlarmilos · 2024-09-27T11:59:03Z

src/Swift.Bindings/src/Model/Provenance.cs

+/// <summary>
+/// Represents the origin of a particular function
+/// </summary>
+public class Provenance {


Isn't this the same property as public required BaseDecl? ParentDecl { get; set; }?

BaseDecl, IIRC is a C# representation not a Swift declaration. Further, extensions make this more complicated because you can have multiple extension on the same type from difference modules.

kotlarmilos · 2024-09-27T11:59:46Z

src/Swift.Bindings/src/Model/SwiftFunction.cs

+/// Represents a Swift function signature and its provenance.
+/// </summary>
+[DebuggerDisplay("{ToString()}")]
+public class SwiftFunction {


Why can't we utilize MethodDecl and it's members instead?

See above. This is strictly Swift not C#.

Add first piece of rule reduction

92f97be

matouskozak reviewed Sep 26, 2024

View reviewed changes

matouskozak approved these changes Sep 26, 2024

View reviewed changes

mcumming reviewed Sep 26, 2024

View reviewed changes

gussying up

8441650

kotlarmilos reviewed Sep 27, 2024

View reviewed changes

kotlarmilos self-requested a review September 27, 2024 12:00

kotlarmilos approved these changes Sep 27, 2024

View reviewed changes

dalexsoto approved these changes Sep 27, 2024

View reviewed changes

stephen-hawley merged commit 3fba66f into feature/swift-bindings Sep 27, 2024
6 checks passed

stephen-hawley deleted the tree-reduction branch September 27, 2024 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Swift] Add first piece of rule reduction #2692

[Swift] Add first piece of rule reduction #2692

stephen-hawley commented Sep 25, 2024

matouskozak Sep 26, 2024

mcumming Sep 26, 2024

stephen-hawley Sep 26, 2024

matouskozak left a comment

matouskozak Sep 26, 2024

matouskozak Sep 26, 2024 •

edited

Loading

matouskozak Sep 26, 2024

mcumming Sep 26, 2024

mcumming Sep 26, 2024

stephen-hawley Sep 26, 2024

mcumming Sep 26, 2024

mcumming Sep 26, 2024

kotlarmilos Sep 27, 2024

stephen-hawley Sep 27, 2024

kotlarmilos Sep 27, 2024

stephen-hawley Sep 27, 2024

	/// The name of this rule - usefule for debugging
	/// The name of this rule - useful for debugging

[Swift] Add first piece of rule reduction #2692

[Swift] Add first piece of rule reduction #2692

Conversation

stephen-hawley commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matouskozak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matouskozak Sep 26, 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

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

matouskozak Sep 26, 2024 •

edited

Loading