renaming of SadlResources in SADL and Dialog

[crapo]

@kittaakos , one of the capabilities of xtext that can be implemented for a DSL is, I believe, the abiltiyh to rename a concept and have the new name appear wherever it is referenced. How hard would it be to implement this for SadlResources in SADL and Dialog? It would be very helpful for this project and in general. Is this something that could be done in a couple of days? If so, how soon could you do it?

[kittaakos]

It should work out of the box. I wanted to verify this, but I could not build it.
I am on MissingPatterns in SADL and on the master branch in Dialog.
Do you know what's causing the problem?

[INFO] Scanning generated classes for implementations...
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] DARPA-ASKE ......................................... SUCCESS [  0.002 s]
[INFO] DARPA-ASKE Dependencies ............................ SUCCESS [  0.624 s]
[INFO] DARPA-ASKE Core .................................... FAILURE [ 34.575 s]
[INFO] DARPA-ASKE IDE ..................................... SKIPPED
[INFO] DARPA-ASKE UI ...................................... SKIPPED
[INFO] DARPA-ASKE Tests ................................... SKIPPED
[INFO] DARPA-ASKE UI Tests ................................ SKIPPED
[INFO] DARPA-ASKE Eclipse Feature ......................... SKIPPED
[INFO] DARPA-ASKE p2 Update Site .......................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:31 min
[INFO] Finished at: 2020-04-15T16:42:37+02:00
[INFO] Final Memory: 146M/1682M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0:generate (default) on project com.ge.research.sadl.darpa.aske.dialog: Execution default of goal eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0:generate failed: An API incompatibility was encountered while executing eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0:generate: java.lang.IncompatibleClassChangeError: com.ge.research.sadl.darpa.aske.parser.antlr.internal.InternalDialogParser and com.ge.research.sadl.darpa.aske.parser.antlr.internal.InternalDialogParser$DFA28 disagree on InnerClasses attribute
[ERROR] -----------------------------------------------------
[ERROR] realm =    plugin>eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0
[ERROR] strategy = org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy
[ERROR] urls[0] = file:/Users/akos.kitta/.m2/repository/eu/somatik/serviceloader-maven-plugin/serviceloader-maven-plugin/1.1.0/serviceloader-maven-plugin-1.1.0.jar
[ERROR] urls[1] = file:/Users/akos.kitta/.m2/repository/org/sonatype/plexus/plexus-build-api/0.0.7/plexus-build-api-0.0.7.jar
[ERROR] urls[2] = file:/Users/akos.kitta/.m2/repository/javax/enterprise/cdi-api/1.0/cdi-api-1.0.jar
[ERROR] urls[3] = file:/Users/akos.kitta/.m2/repository/com/google/guava/guava/10.0.1/guava-10.0.1.jar
[ERROR] urls[4] = file:/Users/akos.kitta/.m2/repository/com/google/code/findbugs/jsr305/1.3.9/jsr305-1.3.9.jar
[ERROR] urls[5] = file:/Users/akos.kitta/.m2/repository/org/sonatype/sisu/sisu-guice/3.1.0/sisu-guice-3.1.0-no_aop.jar
[ERROR] urls[6] = file:/Users/akos.kitta/.m2/repository/aopalliance/aopalliance/1.0/aopalliance-1.0.jar
[ERROR] urls[7] = file:/Users/akos.kitta/.m2/repository/org/eclipse/sisu/org.eclipse.sisu.inject/0.0.0.M5/org.eclipse.sisu.inject-0.0.0.M5.jar
[ERROR] urls[8] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-component-annotations/1.5.5/plexus-component-annotations-1.5.5.jar
[ERROR] urls[9] = file:/Users/akos.kitta/.m2/repository/backport-util-concurrent/backport-util-concurrent/3.1/backport-util-concurrent-3.1.jar
[ERROR] urls[10] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-interpolation/1.11/plexus-interpolation-1.11.jar
[ERROR] urls[11] = file:/Users/akos.kitta/.m2/repository/junit/junit/3.8.1/junit-3.8.1.jar
[ERROR] urls[12] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-utils/3.0.22/plexus-utils-3.0.22.jar
[ERROR] urls[13] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-compiler-api/2.5/plexus-compiler-api-2.5.jar
[ERROR] Number of foreign imports: 1
[ERROR] import: Entry[import  from realm ClassRealm[project>com.ge.research.sadl.darpa.aske.dialog:com.ge.research.sadl.darpa.aske.dialog.parent:1.0.0-SNAPSHOT, parent: ClassRealm[maven.api, parent: null]]]
[ERROR] 
[ERROR] -----------------------------------------------------
[ERROR] 
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginContainerException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :com.ge.research.sadl.darpa.aske.dialog
akos.kitta@Akoss-MacBook-Pro com.ge.research.sadl.darpa.aske.dialog.parent % 

[crapo]

sorry to be imprecise. ASKE TA1 branch should be CodeExtraction for this.

[kittaakos]

I am on that branch, but I still have the same build error. Were there any dependency updates recently?

[crapo]

I have an updated target file for sadlos2 MissingPatterns that I haven't pushed (and I got from you I think) because Alfredo isn't yet on Eclipse 2020-03. Otherwise none that I know of. I don't think I've seen this error.

[crapo]

I have tested Rename element and can report that it doesn't not work.

[kittaakos]

Otherwise none that I know of. I don't think I've seen this error.

Seem to be OK now, I had to wipe the entire repository state locally.

It would be very helpful for this project and in general.

I agree. 👍 It's a good idea.

Is this something that could be done in a couple of days?

I have to check why it does not work out of the box, but I think it should be feasible.

If so, how soon could you do it?

I can start working on this tomorrow, but let me check and get back to you with better time-estimation.

[crapo]

Thanks!!

[kittaakos]

but let me check and get back to you with better time-estimation.

@crapo, it is a non-trivial task. Xtext supports the rename/refactoring in two different ways:

• The default one requires cross-references. SADL grammar does not use cross-references for the SADL resources, so we cannot use it.
• The other approach is based on text-modifications with the change-serializer. This API was introduced in Xtext 2.13, now we are at 2.14, so I am expecting quite a lot of bugs from Xtext itself.

I am going to start the implementation using the latter approach. I will let you know if I run into some unexpected issues, but I still believe it's feasible.

[crapo]

Thank you. Please keep me posted. This would be a significant improvement to SADL usability.

[kittaakos]

Short heads-up: we cannot reuse anything as-is from the Xtext for the renaming. We have to calculate the edit locations manually. I've managed to make some progress, but there is still a lot to code.

Renaming if the declaration is in another file, could be problematic; I will continue with that part to see if there are any show stoppers as soon as possible.

[crapo]

Thank you!

[crapo]

As we move towards our final demo, this renaming would be extremely helpful, even if it only worked within a single .dialog file by the demo freeze, which will be by Wednesday next week. Let me know if focusing on that helps. I judge from the animated image that you may already have that working in some context. Would it work in .dialog files?

[kittaakos]

@crapo, I made some progress but I am not as optimistic as I was yesterday. We can support the followings:

Rename local SADL resource in both SADL and Dialog file:

Rename SADL resource in both SADL and Dialog file, will update SADL resource at declarations-side:

Issues (I am aware of so far):

• We should not allow renaming built-ins,
• We should not allow renaming externals,
• No support for alias rename so far,
• If a SADL resource is defined in a SADL file AND used in more than one Dialog file, for some reason the rename does not work in the Dialog (I have to investigate it),
• We have zero tests,
• During the rename, we update the document which we should not, and as you can see at the second screencast, there is an answer: CM: Concept xxx is not defined; please define or do extraction.

[crapo]

Not sure if this would make it any easier, but it would be acceptable to have to go to the definition to rename. I don't understand the last bullet.
The primary purpose would be that we extract concepts from code comments that are suggested additions to the ontology, and these appear as augmented (semantic) types in the equation signature. We would like the user to be able to rename these concepts (where they are defined in the same dialog file) and have them also be renamed in the augmented type.

[kittaakos]

I created two PRs if you want to experiment with the changes. Nothing else should be affected:

SADL PR against the MissingPatterns branch: SemanticApplicationDesignLanguage/sadl#416
Dialog PR against the CodeExtraction branch: #105

[crapo]

I hate to show my git ignorance, but how do I experiment without merging the PRs? Just go to the branches that you must have created?

[kittaakos]

Just go to the branches that you must have created?

Yes.

In a command line, you fetch (you can pull too), and checkout the branch.

[kittaakos]

• We should not allow renaming built-ins,
• We should not allow renaming externals,

• If a SADL resource is defined in a SADL file AND used in more than one Dialog file, for some reason the rename does not work in the Dialog (I have to investigate it),

I have updated both PRs; the above items are fixed now. 👆

[crapo]

Do I see correctly that you've merged these into MissingPatterns?

[kittaakos]

Do I see correctly that you've merged these into MissingPatterns?

Not at all. The changes are still on the GH-415 branch. Have you had the chance to try it?

[kittaakos]

• We have zero tests,

I have added some basic testing for the rename, but the Dialog content updates break it in some cases; please reference the screencast:

The editor content is updated with:

CM: Concept x is not defined; please define or do extraction.

And it breaks the refactoring workflow.

[crapo]

Humm... this is a little like the discussion about JenaBasedDialogModelProcessor onValidate. The call to AnswerCurationManager processConversation shouldn't happen until the renaming is complete and the new name is resolved. I think I can add logic to detect an invalid query. Would onValidate get called again after the imported ontology is updated and the new name is resolvable?

[kittaakos]

this is a little like the discussion about JenaBasedDialogModelProcessor onValidate

Yes, sort of. In general, we should not update the resource when validating it but trigger the conversation updates on-demand, with another UX feature: #68 (comment). I know, it's out of scope for now but please let's discuss this after the demo.

The call to AnswerCurationManager processConversation shouldn't happen until the renaming is complete and the new name is resolved.

Correct.

I think I can add logic to detect an invalid query.

That would be great!

Would onValidate get called again after the imported ontology is updated and the new name is resolvable?

Yes. It did not work before like that, but I made this happen in #105.

[crapo]

Unfortunately, it isn't that easy.

I think I can add logic to detect an invalid query.

That would be great!

If the concept isn't defined, we want the processConversation call to tell the user that the concept isn't defined. I would need to know that a rename workflow is in-progress in order to just not make the call to processConversation. Is that possible?

[crapo]

I note that renaming a concept in a Dialog editor does not turn on the dirty flag--I guess it doesn't need to be saved? Or is that unexpected?

[kittaakos]

@crapo, I've managed to put together a state which works with my super naive use-cases. I am going to update the PR and prepare it for the merge. Can you please try it, and if you are OK with it, you can integrate the change into the CodeExtraction branch.

Also, you will have the demo tomorrow, right? Do you want to have a quick sync-tcon? If so, please email the link, I am going to be available for another hour or so. Or if you have any other requests, I can work on them.

[crapo]

Tomorrow is the code freeze for the demo on Friday. When the PRs are updated I will try it out. If needed we can synch up then or tomorrow when my day starts if you are available.

[kittaakos]

I have updated both PRs. The changeset is ready if you want to try it again.

• I added a service that tracks if a rename/refactoring is in progress, as far as it can, so we do not update the conversation.
• Please note, if you are undo-ing a rename/refactoring, this newly added service cannot do anything, so you will see CM: Concept x is not defined; please define or do extraction..

Note: I have force pushed to the remote branches!

[crapo]

@kittaakos , this is looking good! I'll do more testing tomorrow, but it seems pretty well behaved for what we want to demo.