Codegen direct index access and map member model #5720
Conversation
| List<RuleType> args = expr.arguments().stream().map(RuleExpression::type).collect(Collectors.toList()); | ||
| if (!function.matches(args)) { | ||
| addError("Arguments for function `%s` doesn't match, expected: `%s`, got: `%s`", | ||
| name, | ||
| function.arguments().values(), args); | ||
| } |
There was a problem hiding this comment.
We need to remove this error check here and above since they will fail when there is two assigns (LetExpression) in a row where second assign depends on the first assigned variable.
Because of the order of execution of the visitors, the first variable will not be stored in SymbolTable.locals before the second assign executes initially. The second assign will execute again successfully after the first variable is stored when super.visitXX is invoked
|
| MemberModel keyModel = memberModel.getMapModel().getKeyModel(); | ||
| MemberModel valueModel = memberModel.getMapModel().getValueModel(); | ||
|
|
||
| b.add("$T.of(", ImmutableMap.class); |
There was a problem hiding this comment.
should we use builder(), since of only supports up to 4 KV pairs?
There was a problem hiding this comment.
I tried to use that initially, but ImmutableMap.builder().build() returns ImmutableMap<K, V>, so its not compatible. We'd have to update build() to return <K, V> ImmutableMap<K, V> like of() does , which would entail an unchecked cast. WDYT?
public <K, V> ImmutableMap<K, V> build() {
HashMap<K, V> builtMap = (HashMap<K, V>) new HashMap<>(entries);
return new ImmutableMap<>(builtMap);
}
There was a problem hiding this comment.
Will merge as is, can update in follow up PR if needed, or can also add new of() overloads to support more pairs if necessary in future
| if ((firstArn = RulesFunctions.listAccess(params.arnList(), 0)) != null) { | ||
| locals = locals.toBuilder().firstArn(firstArn).build(); | ||
| } else { | ||
| return RuleResult.carryOn(); | ||
| } | ||
| if ((parsedArn = RulesFunctions.awsParseArn(locals.firstArn())) != null) { | ||
| locals = locals.toBuilder().parsedArn(parsedArn).build(); |
There was a problem hiding this comment.
I was thinking more along the lines of
if ((firstArn = RulesFunctions.listAccess(params.arnList(), 0)) != null
&& (parsedArn = RulesFunctions.awsParseArn(firstArn)) != null) {
locals = locals.toBuilder().parsedArn(parsedArn).firstArn(firstArn).build();
What you have works I think, just feeling uneasy about being able to reach the end of the second if with locals modified. That doesn't matter now, but before I would've never matter as the side effects would only happen in the if body scope.
There was a problem hiding this comment.
NOTE, No action item or change is expected, just leaving this as a side note.
Nesting could also work leaving the previous semantics, e.g.,
if ((firstArn = RulesFunctions.listAccess(params.arnList(), 0)) != null) {
locals = locals.toBuilder().firstArn(firstArn).build();
if ((parsedArn = RulesFunctions.awsParseArn(locals.firstArn())) != null) {
locals = locals.toBuilder().parsedArn(parsedArn).build();
return RuleResult.endpoint(Endpoint
.builder()
.url(URI.create("https://" + params.endpointId() + ".query."
+ locals.partitionResult().dualStackDnsSuffix()))
.putAttribute(
AwsEndpointAttribute.AUTH_SCHEMES,
Arrays.asList(SigV4aAuthScheme.builder().signingName("query")
.signingRegionSet(Arrays.asList("*")).build())).build());
}
}
return RuleResult.carryOn();There was a problem hiding this comment.
Yeah I also came up with that at first when handwriting it, just not sure how to modify the logic to do so. Need to plumb deeper through the call stack which is already pretty complicated
| if ((firstArn = RulesFunctions.listAccess(params.arnList(), 0)) != null) { | ||
| locals = locals.toBuilder().firstArn(firstArn).build(); | ||
| } else { | ||
| return RuleResult.carryOn(); | ||
| } | ||
| if ((parsedArn = RulesFunctions.awsParseArn(locals.firstArn())) != null) { | ||
| locals = locals.toBuilder().parsedArn(parsedArn).build(); |
There was a problem hiding this comment.
NOTE, No action item or change is expected, just leaving this as a side note.
Nesting could also work leaving the previous semantics, e.g.,
if ((firstArn = RulesFunctions.listAccess(params.arnList(), 0)) != null) {
locals = locals.toBuilder().firstArn(firstArn).build();
if ((parsedArn = RulesFunctions.awsParseArn(locals.firstArn())) != null) {
locals = locals.toBuilder().parsedArn(parsedArn).build();
return RuleResult.endpoint(Endpoint
.builder()
.url(URI.create("https://" + params.endpointId() + ".query."
+ locals.partitionResult().dualStackDnsSuffix()))
.putAttribute(
AwsEndpointAttribute.AUTH_SCHEMES,
Arrays.asList(SigV4aAuthScheme.builder().signingName("query")
.signingRegionSet(Arrays.asList("*")).build())).build());
}
}
return RuleResult.carryOn();
Motivation and Context
Add support for parsing direct index access in endpoint rules set, e.g.,
[123]Add support for map MemberModel in client endpoint tests
Testing
Added codegen tests
Builds pass locally for new feature