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

Preserve identifiers when reconstructing AstMethod #1089

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

jasmith-hs
Copy link
Contributor

An aliased macro function (aka a local macro function) can be defined which modifies the value of a non-primitive argument. These are parsed by the Ast parser as AstMethod rather than AstMacroFunction. The AstMethod and AstMacroFunction should reconstruct parameters in the same way so I am making that consistent, by actually using the EagerAstParameter's definition of how to reconstruct itself.

Another change I am making is defining a IdentifierPreservationStrategy enum which lets a DeferredParsingException know if it is thrown by one of these classes which forces preserving identifiers (this is done because of the possibility of modifying a variable. Modifying [] is different than modifying my_list, which starts as []). By adding this information, we know that the partially resolved value within the DeferredParsingException is done such that the necessary identifiers have been preserved. Previously, if preserveIdentifer == true, we'd ignore the result of the DeferredParsingException, and remake it which led to duplicate work and with the other change I'm making in this PR, would not allow the macro function temp variable logic to work.

These new tests demonstrate how we need to preserve foo_map, imagining that this .modify method does some modification on foo_map:

@Test
public void itPreservesNonDeferredIdentifier() {
  try {
    interpreter.resolveELExpression("deferred.modify(foo_map)", -1);
    fail("Should throw DeferredParsingException");
  } catch (DeferredParsingException e) {
    assertThat(e.getDeferredEvalResult()).isEqualTo("deferred.modify(foo_map)");
  }
}

@Test
public void itPreservesNonDeferredIdentifierWhenSecondParamIsDeferred() {
  try {
    interpreter.resolveELExpression("foo_list.modify(foo_map, deferred)", -1);
    fail("Should throw DeferredParsingException");
  } catch (DeferredParsingException e) {
    assertThat(e.getDeferredEvalResult())
      .isEqualTo("foo_list.modify(foo_map, deferred)");
  }
}

…nction.

Also migrate both to directly use the EagerAstParameters.getPartiallyResolved to reconstruct the parameters. There's no reason that it wasn't already done this way
…rred.

To do this, we need to be aware of if the DeferredParsingException was created when forcing the identifiers to be preserved.
So I introduced a new enum to be more readable than the boolean that represented that before
@jasmith-hs jasmith-hs merged commit 69379e5 into master Jul 13, 2023
@jasmith-hs jasmith-hs deleted the preserve-identifier-in-method branch July 13, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant