Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ambiguous annotation values by checking an additional flag #27

Merged
merged 4 commits into from
Jan 29, 2023

Conversation

thekid
Copy link
Member

@thekid thekid commented Jan 29, 2023

Problem

For BC reasons, xp::$meta contains annotation values as follows:

Declaration Value Remark
#[Test] null 0️⃣ Empty arguments becomes NULL
#[Author('Timm')] 'Timm' 1️⃣ Exactly one argument emitted as itself
#[Values(1, 2)] [1, 2] ➕ Multiple arguments emitted as array

(See https://github.com/xp-framework/compiler/blob/v8.8.0/src/main/php/lang/ast/emit/php/XpMeta.class.php#L27 and following)

However, this is a problem when the first argument is an array or NULL, as the following become ambiguous:

Declaration Value Remark
#[Values([1, 2])] [1, 2] 1️⃣ Exactly one
#[Values(1, 2)] [1, 2] ➕ Multiple
#[Values(null)] null 1️⃣ Exactly one
#[Values] null 0️⃣ Empty

For the values logic currently in use, this is not a problem. However, when using newInstance() with annotation classes, the desired outcomes are different from reality:

class Values {
  public function __construct(private string|iterable $source) { }

  public function arguments($type): iterable {
    return is_string($this->source)
      ? $type->method($this->source)->invoke(null)
      : $this->source
    ;
  }
}

// Intended usage:
#[Values([1, 2])]     // Expect: new Values([1, 2]), yield 1 and 2 as values
#[Values('provider')] // Expect: new Values('provider'), yield from self::provider()

The first syntax currently invokes the constructor with $list= 1 and an excess argument which is silently dropped, causing type checker errors - as now we're using the arguments logic! Even wors, when using #[Values(['test'])], instead of yielding the value test, a method named test is located on the given type, which is completely unintuitive behavior!

This pull request changes reflection code to check for an additional flag for the exactly one argument case, check it and wrap the meta value in an array, resolving the ambiguity, but retaining backwards compatibility:

Declaration Value (XP Core) Arguments (XP Reflection)
#[Values([1, 2])] [1, 2] [[1, 2]] 🆕
#[Values(1, 2)] [1, 2] [1, 2]
#[Values(null)] null [null] 🆕
#[Values] null []
#[Values(1)] 1 [1] (unchanged!)

The last row is unchanged because of how the (array) cast works on scalar values!

Dependencies

For this to work end-to-end a small change to XP Compiler is required:

diff --git a/src/main/php/lang/ast/emit/php/XpMeta.class.php b/src/main/php/lang/ast/emit/php/XpMeta.class.php
index 0da5bcc..9e35b3b 100755
--- a/src/main/php/lang/ast/emit/php/XpMeta.class.php
+++ b/src/main/php/lang/ast/emit/php/XpMeta.class.php
@@ -29,6 +29,7 @@ trait XpMeta {
       } else if (1 === sizeof($arguments) && isset($arguments[0])) {
         $this->emitOne($result, $arguments[0]);
         $result->out->write(',');
+        $lookup[$name]= 1;
       } else {
         $result->out->write('[');
         foreach ($arguments as $name => $argument) {

Outlook

This implementation goes around several corners to be free of BC breaks. With the removal of reflection from XP core suggested in xp-framework/rfc#338, we also have the chance of redefining how xp::$meta is to be used.

@thekid thekid merged commit dc347e7 into main Jan 29, 2023
@thekid thekid deleted the fix/ambiguous-annotation-values branch January 29, 2023 14:25
thekid added a commit to xp-framework/compiler that referenced this pull request Jan 29, 2023
@thekid
Copy link
Member Author

thekid commented Jan 29, 2023

XP Compiler patch released in https://github.com/xp-framework/compiler/releases/tag/v8.8.1

@thekid thekid changed the title Fix ambiguous annotation values by checking an additional meta value Fix ambiguous annotation values by checking an additional flag Jan 29, 2023
@thekid
Copy link
Member Author

thekid commented Jan 29, 2023

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.

1 participant