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

Default value fixes in M3 compiler and reactivator #827

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

aziemchawdhary-gs
Copy link
Contributor

@aziemchawdhary-gs aziemchawdhary-gs commented Jun 4, 2024

  1. Fixes a bug in the compiled mode version of reactivator where evaluation of calls to new does not set default values defined on the class being instantiated. More specifically this bug occurs when the reactivator doesn't need to regenerate and compile Java code and can evaluate the call to new directly.

  2. Fails compilation on association properties with default values. This was not supported in compiled mode and a new compiler check is added to not allow default values on association properties

Copy link

github-actions bot commented Jun 4, 2024

Test Results

   378 files     378 suites   1h 19m 50s ⏱️
3 567 tests 3 544 ✔️ 23 💤 0
5 139 runs  5 116 ✔️ 23 💤 0

Results for commit b1a40ae.

♻️ This comment has been updated with latest results.

@aziemchawdhary-gs aziemchawdhary-gs marked this pull request as ready for review June 4, 2024 21:30
@aziemchawdhary-gs aziemchawdhary-gs marked this pull request as draft June 6, 2024 20:35
@aziemchawdhary-gs aziemchawdhary-gs marked this pull request as ready for review June 10, 2024 15:04
@aziemchawdhary-gs aziemchawdhary-gs marked this pull request as draft June 11, 2024 13:13
@aziemchawdhary-gs aziemchawdhary-gs changed the title Compiled mode: set default values for new instances in reactivator flow Default value fixes in M3 compiler and reactivator Aug 8, 2024
@aziemchawdhary-gs aziemchawdhary-gs force-pushed the default_value_reactivator branch from 7937efb to ca7de4f Compare August 29, 2024 20:19
@aziemchawdhary-gs aziemchawdhary-gs marked this pull request as ready for review August 29, 2024 20:20
@aziemchawdhary-gs aziemchawdhary-gs force-pushed the default_value_reactivator branch from ca7de4f to dea7e4b Compare November 2, 2024 14:27
@@ -721,13 +723,17 @@ public static Object newObject(Bridge bridge, org.finos.legend.pure.m3.coreinsta
{
Class<?> c = ((CompiledExecutionSupport) es).getClassLoader().loadClass(JavaPackageAndImportBuilder.platformJavaPackage() + "." + Pure.elementToPath(aClass, "_", true) + "_Impl");
Any result = (Any) c.getConstructor(String.class).newInstance(name);
MutableList<String> userAssignedParameters = Lists.mutable.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MutableList<String> userAssignedParameters = Lists.mutable.empty();
MutableSet<String> userAssignedParameters = Sets.mutable.empty();

Comment on lines +749 to +772
RichIterable<CoreInstance> classProperties = _Class.getSimpleProperties(aClass, ((CompiledExecutionSupport) es).getProcessorSupport());
classProperties.forEach(new CheckedProcedure<CoreInstance>()
{
@Override
public void safeValue(CoreInstance p) throws Exception
{
if (!userAssignedParameters.contains(p.getName()))
{
Property<?, ?> prop = (Property<?, ?>) p;
DefaultValue defaultValue = prop._defaultValue();
if (defaultValue != null)
{
Object res = reactivate(defaultValue._functionDefinition()._expressionSequence().getFirst(), new PureMap(Maps.fixedSize.empty()), bridge, es);
Method method = c.getMethod("_" + prop._name(), RichIterable.class);
if (res instanceof RichIterable)
{
method.invoke(result, res);
}
else
{
method.invoke(result, Lists.fixedSize.of(res));
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have methods now on AbstractCompiledCoreInstance for dealing with default values. Maybe result above can be cast directly to AbstractCompiledCoreInstance? In case it needs to be cast to Any above, I'm casting it in the suggestion below.

Suggested change
RichIterable<CoreInstance> classProperties = _Class.getSimpleProperties(aClass, ((CompiledExecutionSupport) es).getProcessorSupport());
classProperties.forEach(new CheckedProcedure<CoreInstance>()
{
@Override
public void safeValue(CoreInstance p) throws Exception
{
if (!userAssignedParameters.contains(p.getName()))
{
Property<?, ?> prop = (Property<?, ?>) p;
DefaultValue defaultValue = prop._defaultValue();
if (defaultValue != null)
{
Object res = reactivate(defaultValue._functionDefinition()._expressionSequence().getFirst(), new PureMap(Maps.fixedSize.empty()), bridge, es);
Method method = c.getMethod("_" + prop._name(), RichIterable.class);
if (res instanceof RichIterable)
{
method.invoke(result, res);
}
else
{
method.invoke(result, Lists.fixedSize.of(res));
}
}
}
AbstractCompiledCoreInstance instance = (AbstractCompiledCoreInstance) result;
instance.getDefaultValueKeys().forEach(new CheckedProcedure<String>()
{
@Override
public void safeValue(String p) throws Exception
{
if (!userAssignedParameters.contains(p))
{
RichIterable<?> defaultValues = instance.getDefaultValue(p, es);
if ((defaultValues != null) && defaultValues.notEmpty())
{
Method method = c.getMethod("_" + prop._name(), RichIterable.class);
method.invoke(result, defaultValues);
}
}

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.

2 participants