Pretty-printing bugfix on renaming

[AZWN]

Thnx!

To be totally sure it works, I'll try a local build first. Probably, we also need to wrap the constructed scope graphs into this new constructor then.

[hendrikvanantwerpen]

This change is not as innocent as it looks, indeed.

At least the following places need to be adapted as well:

• https://github.com/metaborg/nabl/blob/master/statix.solver/src/main/java/mb/statix/spoofax/STX_debug_scopegraph.java#L91
• https://github.com/metaborg/nabl/blob/master/statix.solver/src/main/java/mb/statix/spoofax/StatixTerms.java#L628
• https://github.com/metaborg/nabl/blob/master/statix.solver/src/main/java/mb/statix/spoofax/StatixTerms.java#L570
• All the jdk8.stxlib files in the nabl, java-front, java-evaluation, and statix-benchmark repositories need to be replaced with a newly generated version.

Perhaps it is still worth it do so, since having constructors is usually better—if only for more easily finding places where that constructor is used. But we should review these changes thorougly before merging.

@pmisteliac If I remember correctly, you are proposing this change because of some pretty-printing problems you have. Are you using prettyprint-Statix or prettyprint-SDF-start-symbols by any chance? Using these can easily cause weird behaviour, as it will choose the first rule that matches, and does not really work well with constructor-less rules, but even overloaded constructor names can cause problems. In that case it is better to use a specific strategy, such as prettyprint-Statix-Start, for example.

[pmisteliac]

I use construct-textual-change-statix = construct-textual-change(pp-partial-Statix-string, parenthesize, override-reconstruction, resugar) to pretty print the file. I just saw that the statix.lang project has to pp.str modules so maybe I've been using the wrong one?

That change you propose looks quiet complex for me as I only have a basic understanding of pretty printing. Do you think it could even fix my problem?

[AZWN]

I think the problem should be solvable, but probably we need to add more constructors than only this one (although I'm still a bit surprised it does not work).

I can help you with the changes in the Java code, so that should not be a problem. Also, we might make the scope-graph loading lenient (accepting both the old and the new format), saving the regeneration of the libraries for now.

[hendrikvanantwerpen]

Also, we might make the scope-graph loading lenient (accepting both the old and the new format), saving the regeneration of the libraries for now.

That is a good solution to make the migration easier, indeed.

[AZWN]

Ok, so shortly summarizing the status: Currently, the renaming algorithm in the Statix language prepends scope graph for every pretty-printed position (renamed identifiers, and the whole program). This is due to the fact that pp-partial-Statix-string (which is used by the layout-preserving pretty-print strategy construct-textual-change) is unreliable wrt. the actual pp-strategy that is chosen, because there are no constructor names that force deterministic selection of the appropriate rule.

Just adding the ScopeGraph constructor name did not fix the issue for me locally, so we probably will need to add more constructor names. To be more robust, I also think we should solve the duplicate constructor names (either by renaming or indirection & injection).

Finally, we need to adapt all transformation and Java term generation to the new AST structures. Also, the Java code that loads terms (mostly scope graphs, but when renaming is needed, also constraints) would need to be adapted.

@pmisteliac it would be great if you could contribute a minimal set of additional constructor names that would make the pretty-printing with construct-textual-change work. When those are in place, I can contribute the adaptions to the Java code.

[pmisteliac]

Your observation is correct. Noteworthy is that the problem only seems to occur when Statix is loaded from the Eclipse plugin. If you build it locally in your Spoofax instance the problem is not reproducible and the renaming works as intended.

I'll try and get the grammar fixed this weekend. Thanks for your help guys!

[pmisteliac]

I added the constructors with some hopefully sensible names and got rid of all the warnings in the syntax files. Testing with a local build worked fine, but then again so it did last time.

[AZWN]

Ok, last time I caught it with a local build. I probably can try this evening. After that, could you give me permission to your fork to adapt the Java side of the story?

[AZWN]

This last push contains several things:

• First of all, I discovered that my local build was incorrect. Testing again showed that adding the ScopeGraph constructor was already sufficient. Therefore I reverted the last commit.
• I implemented the necessary adaptions on the Java side.
• I tested the following use cases:
  • [x] Renaming some constructs in a regular Statix file
  • [x] Renaming some constructs in a Statix test file
  • [x] Pretty-printing a scope graph for a project from the menu (Show Project Scope Graph)
  • [x] Evaluating a test
  • [x] Execute all 'Format XXX AST' menu items.
  • [x] Executing the parallel solver with a library in the old format
  • [x] Executing the parallel solver with a library in the new format, generated by the jar-analyzer tool
  • [x] Inspecting a library in the new format, generated by the 'Make project library' menu option.
• Besides that, I did some greping on the repo, and I think the Java locations @hendrikvanantwerpen pointed out are all we need to cover now.

@pmisteliac To be totally sure, could you go through (at least) the first five test cases as well? @hendrikvanantwerpen Do you think there are use cases missing? Otherwise, do you think we can merge this?

[pmisteliac]

I did execute the 5 first test cases on a local build and it all seems to work fine :tada: I created a new eclipse workspace and only imported statix.example and statix.test.

[pmisteliac]

Also tested it with the Statix specification of WebDSL which is quiet large. All seems to work properly now

[hendrikvanantwerpen]

This looks alright to me. The last release was enough for my artifact, so feel free to merge as you see fit @AZWN.

[AZWN]

Great! I'll process the SCOPEGRAPH_OP review comment, and then merge it afterwards.

[pmisteliac]

I just tested it again with the build from the jenkins server and it works as well :+1: