Drop Quotable type

[mcimadamore]

The Quotable type is a marker interface that is used to mark lambda expressions and method reference for which javac should produce a code model (we call them quotable lambdas), as in:

Runnable r = (Runnable & Quotable)() ->  {}

Since the lambda metafactory, nor Op::opQuotable only rely on this type in a superficial way, this PR simplifies the programming model by removing Quotable and use the @CodeReflection annotation (now turned into a type annotation) to mark quotable lambdas, as in:

Runnable r = (@CodeReflection Runnable)() ->  {}

Supporting this is relatively straightforward. The most complex part is, perhaps, to make sure that (a) the type annotation we care about is visible in the AST (javac is very lazy when it comes to fill in information on type annotations) and (b) make sure we don't end up using type annotations that appear in positions other than a cast. The latter point is important; if we have this:

void m(@CodeReflection Runnable r)  { ... }
m(() -> { .. });

we don't want the lambda to magically become quotable simply because the target method parameter has been annotated.

There's also other cases like:

@CodeReflection
Runnable m()  {
   return () -> { .. };
}

If @CodeReflection is a type annotation, the above lambda has a target @CodeReflection Runnable and is also treated as a quotable lambda. What we want here instead is for @CodeReflection to just act as a regular declaration annotation on the method.

To address these issues, I've augmented both ReflectMethods and its associated BodyScanner to keep track of the prviously visited node. This is done by having both classes extend a new simple scanner TreeScannerPrev, which "remembers" the node visited before the current one.

Thanks to this, when we see a lambda we can see if the previous node was a cast, and if such a cast contained any type-annotated type. If so, we will treat the lambda as a quotable lambda.

This implementation tactic guarantees that type annotations will only be consulted when needed.

---

Progress

• Change must not contain extraneous whitespace

Reviewers

• Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to b0b3fe35

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/685/head:pull/685
$ git checkout pull/685

Update a local copy of the PR:
$ git checkout pull/685
$ git pull https://git.openjdk.org/babylon.git pull/685/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 685

View PR using the GUI difftool:
$ git pr show -t 685

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/685.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into code-reflection will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

Drop Quotable type

Reviewed-by: psandoz

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the code-reflection branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the code-reflection branch, type /integrate in a new comment.

[mcimadamore]

There's some future work in this area, not addressed in this PR:

• (cosmetic) we plan to rename the @CodeReflection annotation to just @Reflect
• we want to tweak the logic for detecting quotable lambdas and make it stricter, so that both an annotation at the use site, and one at the declaration side is needed. E.g.

@Reflect
interface QuotableRunnable extends Runnable { }

QuotableRunnable r = (@Reflect QuotableRunnable) () -> { ... }

This is a fussier model, but one that makes sure that reflectability of a lambda expression is always controlled by the client.

[mcimadamore]

Some adjustments were needed to make sure the type annotations were initialized correctly up to this point. I verified with our annotation processor in crExamples and everything still works.

[mcimadamore]

Note: BodyScanner also defines a currentNode field to keep track of the node being visited. Unfortunately, this field is also used outside the scanning process -- e.g. before (at construction) and after (to finish up things). This means it is currently very hard to merge this field with TreeScannerPrev.currentNode. Maybe this is something we can look again in a separate PR.

[mcimadamore]

Ok, NVM, I think I found a good way to consolidate the code and get rid of that field.

[mcimadamore]

I've dropped the TreeTranslator supertype here, as it seems to me ReflectMethods doesn't need it; the only thing ReflectMethod does is generating models for quotable methods and lambdas. The models are stored in appropriate AST nodes, but we don't need to alter the structure of the AST (at least for now).

[mcimadamore]

This was dead code

[mcimadamore]

This was used, but I think it was added long ago, before we cleaned up the compiler pipeline as part of flexible constructor bodies.

[mcimadamore]

These scan methods are tricky. When scanning a quotable method, currentNode points to the method decl (a JCMethodDecl node)., and body points at the method body (a JCBlock node).

But when scanning quotable lambdas, both currentNode and body point at a JCLambda. Hence the different "previous node" forced on the TreeScannerPrev::scan.

[mlbridge]

Webrevs

• 04: Full (dfb94452)
• 03: Full - Incremental (b8da5394)
• 02: Full - Incremental (b0b3fe35)
• 01: Full - Incremental (87c65f24)
• 00: Full (2cb39a5c)

[mcimadamore]

Note: while I have updated HAT code, I have not tested that it builds/works correctly.

[mcimadamore]

Note: while I have updated HAT code, I have not tested that it builds/works correctly.

Actually, I was able to build -- but not to test as I do not have opencl setup here.

[PaulSandoz]

This looks good (one minor comment about possibly redundant code check in the code reflection tester).

In a subsequent PR we can rename @CodeReflection to @Reflect and replace use of the term Quotable with Reflectable (we need to keep Quoted, its a more general concept and i cannot think of a better name right now.)

[PaulSandoz]

Do we need the above check if (field.getType().equals(Quoted.class)) ...?

[openjdk]

@mcimadamore this pull request can not be integrated into code-reflection due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout quotable_anno
git fetch https://git.openjdk.org/babylon.git code-reflection
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge code-reflection"
git push

[PaulSandoz]

I am balking on following up and requiring use-site and declaration-site alignment:

@FunctionalInterface
@Reflect
ReflectableRunnable extends Runnable {}

void m(ReflectableRunnable r) {
}

// Fail, use site and declaration site are not aligned
m(() -> {}) 

// Pass
m((@Reflect ReflectableRunnable) () -> {}) 

I worry the current verbosity is so unpalatable it will reduce folks willingness to experiment. We will somehow need to get there eventually with some new concise language feature(s).

[mcimadamore]

/integrate

[openjdk]

Going to push as commit 577c89e.

[openjdk]

@mcimadamore Pushed as commit 577c89e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.