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

ST6RI-770 Computing the derived property View::exposedElement causes an exception #565

Merged
merged 4 commits into from
May 9, 2024

Conversation

seidewitz
Copy link
Member

@seidewitz seidewitz commented May 9, 2024

This PR fixes a bug that was introduced in PR #554 (Implement invocation delegates for operations).

The Import_importedMembers_InvocationDelegate handles the implementation of the Import::importedMembers operation for all concrete specializations of the abstract metaclass Import. Unfortunately, the class Import_importedMembers_InvocationDelegate was also left abstract in PR #554, which means it could not be instantiated when the operation was invoked. The bug presented itself in a failure to compute the derived value for the ViewUsage::exposedElement property, whose implementation calls importedMembers on Expose relationships, which are kinds of Import relationships (this happens, e.g., when doing a %publish or %view in Jupyter on a view usage with an expose).

This PR includes the following changes:

  1. Makes Import_importedMembers_InvocationDelegate class concrete, which resolves the reported bug.
  2. Slightly refactors the SysML grammar productions for Expose to avoid a possible ParseException on an ill-formed expose declaration.
  3. Updates the compiler and JRE settings for various Eclipse projects to ensure they are all consistent with Java 17.

Note. Since the 2024-04 release has already been tagged, the changes from this PR will be included in an incremental 2024-04.1 release.

- Also added a test for the ViewUsage::exposedElement derived property
computation to demonstrate that the reported bug is fixed.
@seidewitz seidewitz requested a review from himi May 9, 2024 17:24
@seidewitz seidewitz self-assigned this May 9, 2024
@seidewitz seidewitz added this to the 2024-04 milestone May 9, 2024
@seidewitz seidewitz requested a review from arminzavada May 9, 2024 17:30
@himi
Copy link
Member

himi commented May 9, 2024

I'm testing this but I could not reproduce this problem with your test case:

package Test {
 	package P {
 		public part p1;
 	}
 
 	view v {
		expose P::*;
	}
}

was parsed without an error: Package Test (3f7207c9-87e8-4eca-9253-56572e8c47ad)
However,

package Test {
 	package P {
 		public part p1;
 	}
 
 	view v {
		expose P::*::*;
	}
}

caused an exception you mentioned.

org.eclipse.xtext.parser.ParseException: java.lang.IllegalArgumentException: Cannot create instance of abstract class 'Expose'
	at org.eclipse.xtext.parser.antlr.AbstractAntlrParser.doParse(AbstractAntlrParser.java:106)
	at org.eclipse.xtext.parser.antlr.AbstractAntlrParser.parse(AbstractAntlrParser.java:85)
	at org.eclipse.xtext.parser.antlr.AbstractAntlrParser.doParse(AbstractAntlrParser.java:63)
	at org.eclipse.xtext.parser.AbstractParser.parse(AbstractParser.java:34)
	at org.eclipse.xtext.resource.XtextResource.doLoad(XtextResource.java:178)
	at org.eclipse.xtext.linking.lazy.LazyLinkingResource.doLoad(LazyLinkingResource.java:114)
	at org.eclipse.xtext.resource.XtextResource.reparse(XtextResource.java:215)
	at org.omg.sysml.interactive.SysMLInteractive.parse(SysMLInteractive.java:169)
	at org.omg.sysml.interactive.SysMLInteractive.process(SysMLInteractive.java:209)
	at org.omg.sysml.interactive.SysMLInteractive.process(SysMLInteractive.java:203)
	at org.omg.sysml.jupyter.kernel.SysMLKernel.eval(SysMLKernel.java:102)
	at io.github.spencerpark.jupyter.kernel.BaseKernel.handleExecuteRequest(BaseKernel.java:334)
	at io.github.spencerpark.jupyter.channels.ShellChannel.lambda$bind$0(ShellChannel.java:64)
	at io.github.spencerpark.jupyter.channels.Loop.lambda$new$0(Loop.java:21)
	at io.github.spencerpark.jupyter.channels.Loop.run(Loop.java:78)

So let me suggest to change the xpect test case.

@seidewitz
Copy link
Member Author

That's a different exception. expose P::*::*; is not legal syntax. Though that should really be reported as an error, not an exception.

The bug was not in parsing your first example. It was in calling the importedMembership operation on an Expose relationship or computing the exposedElement derived property. This cannot be tested in Xpect, because this property is not used in any validation. I added a JUnit test to org.omg.sysml.interactive.test.DerivedPropertyTest to test it programmatically.

The bug was discovered by trying to %publish the model from Jupyter, because, in this case, all derived properties are computed. If you try this in the current JupyterHub deployment, you will see the exception. It will also happen if you try to do a %view on the view usage. If you install a local Jupyter instance from this branch, the exception will not occur.

@himi
Copy link
Member

himi commented May 9, 2024

So is that the exception you mentioned?
Screenshot 2024-05-09 at 5 17 08 PM

@seidewitz
Copy link
Member Author

seidewitz commented May 9, 2024

Yes. The exception trace and steps to reproduce are in the Jira bug report, but I didn't all of that into the PR description.

@himi
Copy link
Member

himi commented May 9, 2024

I checked it in ST6RI-770 branch and confirmed it worked:
Screenshot 2024-05-09 at 5 29 56 PM

However, as you implied, the parsing problem is not solved because it's different issue. I hope it should be fixed as well because exception is not a correct result for syntax error. But I think it's not urgent.

@himi
Copy link
Member

himi commented May 9, 2024

Just note that %publish was worked without an error:

Screenshot 2024-05-09 at 5 34 13 PM

@seidewitz
Copy link
Member Author

The parse exception seems to be a problem with the ANTLR grammar generated by Xtext. The odd thing is that the problem does not happen for Import, which has almost the same syntax. I slightly refactored the productions for Expose so they were exactly like Import, and the problem went away. So, I am going to go ahead and include this update in the PR, too.

@himi
Copy link
Member

himi commented May 9, 2024

Thank you. I confirmed that showed an error message instead of throwing an exception:
Screenshot 2024-05-09 at 6 14 29 PM

@seidewitz seidewitz merged commit 6875155 into master May 9, 2024
2 checks passed
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.

None yet

2 participants