Skip to content

Conversation

@cirras
Copy link
Collaborator

@cirras cirras commented May 13, 2025

This PR fixes a significant grammar ambiguity between attributes and interface GUIDs.

Consider the following:

const
  FooAttribute = class(TCustomAttribute)
  end;

  TBar = interface
    [Foo]
    procedure Baz;
  end;

At the moment, the grammar interprets [Foo] as a GUID containing constant expression Foo.
It should actually be interpreted as a custom attribute on the Baz procedure.

In the process of resolving this ambiguity, I discovered some awful details.

const
  FooAttribute = class(TCustomAttribute)
  end;

  TBar = interface
    [Foo, '{4E89BDD0-B67C-4B4D-86E6-2C0D1DC7B237']
    procedure Baz;
  end;

This applies FooAttribute to Baz and applies the GUID to TBar.
Based on this, it's clear that the custom attribute and GUID syntax are really one and the same, and the ambiguity was a result of us pretending they were different nodes in the first place.

There's some gnarly technical details here about who should actually owns the attribute node in a case like this.

Here's how it shook out:

  • Any AttributeListNode as the first child of an InterfaceTypeNode is owned by that parent type node.
  • The first declaration (routine/property) within the InterfaceTypeNode must look upward to find that attribute list in RoutineHeadingNode::getAttributeList and PropertyNode::getAttributeList.
  • There is no further disambiguation of ownership, even if we can statically determine that the final attribute of the first attribute group of the attribute list is a GUID.

This would have been easier to handle with #261.

@cirras cirras requested a review from fourls May 13, 2025 03:33
Copy link
Collaborator

@fourls fourls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - missing a couple of changelog entries.

@cirras
Copy link
Collaborator Author

cirras commented May 14, 2025

  • Any AttributeListNode as the first child of an InterfaceTypeNode is owned by that parent type node.
  • The first declaration (routine/property) within the InterfaceTypeNode must look upward to find that attribute list in RoutineHeadingNode::getAttributeList and PropertyNode::getAttributeList.
  • There is no further disambiguation of ownership, even if we can statically determine that the final attribute of the first attribute group of the attribute list is a GUID.

I've decided that I really hate what this does to the structure of the AST, so I've made changes to the ownership situation.
Instead of InterfaceTypeNode always owning the first AttributeListNode, it now only owns it if there are no member declarations.

This keeps all of the annoying AttributeListNode lookup complexity encapsulated to InterfaceTypeNodeImpl (it's either a child or on the first member), and makes the AST structure more consistent.

I also ran across some bugs:

  • RoutineName rule was raising issues on the entire routine declaration node (including attributes)
  • RoutineNameDeclaration::attributeTypes were always unknown for implementation-location routines.
  • PropertyNameDeclaration::attributeTypes were always unknown.

@cirras cirras requested a review from fourls May 14, 2025 02:00
fourls
fourls previously approved these changes May 14, 2025
Copy link
Collaborator

@fourls fourls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The restructure makes this a LOT better.

@fourls
Copy link
Collaborator

fourls commented May 14, 2025

Happy to merge after a rebase to fix changelog conflicts.

cirras added 3 commits May 14, 2025 13:48
Specifically...
- `PropertyNameDeclaration::attributeTypes` were always unknown.
- `RoutineNameDeclaration::attributeTypes` were unknown for
   implementation-local routines.
@fourls fourls merged commit 8bbb1b1 into master May 14, 2025
4 checks passed
@fourls fourls deleted the ambiguous_guid branch May 14, 2025 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants