Base implementation of Dynamix, Tim #111
Conversation
|
|
||
| @DynamixRuntimeScope | ||
| public class DynamixRuntimePrettyPrint implements TaskDef<IStrategoTerm, Result<String, ?>> { | ||
| private final Provider<StrategoRuntime> strategoRuntimeProvider; |
There was a problem hiding this comment.
Using DynamixRuntimeGetStrategoRuntimeProvider here results in an exception in PIE on every build after the first, due to reasons I don't fully understand. For now this is acceptable, since the stratego ctree/jar of the runtime will be bundled with Eclipse anyway, but we should likely get a proper dependency on it.
There was a problem hiding this comment.
Can you add a TODO in the code here that we should switch to DynamixRuntimeGetStrategoRuntimeProvider if possible and the reason this was not done?
There was a problem hiding this comment.
Very cool, and also very quickly done!
Are there any examples and/or regression tests for dynamix, to ensure that dynamix will not suddenly break? If not, it would be good to at least have some simple smoke tests.
Answers to general questions:
The ReadDynamixSpecTaskDef is possibly a bit over the top and could be changed to just passing a ResourcePath, and having the reading be done in the dynamix runtime instead, but I wasn't entirely sure if that would work properly with PIE.
Maybe the reading can be done by the dynamix runtime instead of outputting an AST from a task (since reading is probably so fast that caching may not be useful?). However, for incrementality you must still tell PIE about the read (require) dependency, otherwise changing the specification will not result in rerunning the dynamix runtime.
Menu items are included for running the Dynamix specification. However, the continuous versions of these seem to not work. Possibly due to the fact that the output is written to a file.
Hmm, this could be a bug, can you make a bug report so I can look into this sometime?
Embedding dynamix_runtime should be complete (largely copied from the rv32im code), but I'm not entirely certain if is correct. I am not sure whether a standalone eclipse build will work.
From a glance it looks ok, but can you test this? If you run gradle buildSpoofax3LwbEclipseInstallation in devenv it will create a standalone installation somewhere in spoofax.pie/lwb.distrib/spoofax.lwb.eclipse.repository/build/ (either in eclipse-<os>-<arch> or in the distributions directory. If there are problems we can look into it.
Passing options to the DynamixRuntime is currently done through createDynamixRuntimeConfig in ExecuteDynamixSpecificationTaskDef.java.mustache. This works, but there might be a more idiomatic approach?
That's what we do in other places too, it's easy and works. The downside is that changing configuration requires recompilation of the Java classes. You could also write the options to a Java properties file during language compilation (be sure to use mb.common.util.Properties#storeWithoutDate to ensure no date is written to make it deterministic) and require and read that in the task.
The dynamix_runtime "language" is currently in the metalang folder, but it might be more appropriate to put it elsewhere (next to rv32im?).
It needs to be part of the lwb composite build dependency wise, so it should stay there. Maybe there could be another metaruntime directory for projects like that.
The tim stratego interpreter is currently bundled with the metalang (and not split into tim_runtime or something similar). Currently done because it works and as mentioned this shouldn't be the long-term solution for running tim files.
I guess that is fine.
Source imports for ESV files don't seem to work. Try uncommenting the tim/colors import in the dynamix Main.esv.
Can you make a bug report for this as well, so I can have a look at it sometime?
(Spoofax 2) versions of all the languages are set to 0.1.0-SNAPSHOT. Is that fine?
I think the Spoofax 2 Gradle plugin will override that with the version from Gradle, so that should be fine.
All stratego files have commented out type signatures for str2. Ideally the files are ported over the str2, but I had some issues with getting Stratego 2 to work properly in Spoofax 2 so we're back on the old stratego for now.
That is too bad, but fine solution for now. Did you contact Jeff or make a bug report?
|
|
||
| @DynamixRuntimeScope | ||
| public class DynamixRuntimePrettyPrint implements TaskDef<IStrategoTerm, Result<String, ?>> { | ||
| private final Provider<StrategoRuntime> strategoRuntimeProvider; |
There was a problem hiding this comment.
Can you add a TODO in the code here that we should switch to DynamixRuntimeGetStrategoRuntimeProvider if possible and the reason this was not done?
| specAst = readTerm(inputStream); | ||
| } | ||
| return Result.ofOk(specAst); | ||
| } catch(Exception ex) { |
There was a problem hiding this comment.
Catching all Exceptions is bad practice, as it also catches all RuntimeExceptions indicating bugs such as NullPointerException which should just crash the build, as well as catching InterruptedExceptions which should cancel the build, etc.
Can you catch a more specific exception? If not, can you rethrow RuntimeException and InterruptedException?
| try { | ||
| return reader.parseFromStream(stream); | ||
| } catch(IOException e) { | ||
| throw new IllegalStateException("Loading ATerm from stream failed unexpectedly", e); |
There was a problem hiding this comment.
Can the IOException be handled in a nicer way? Either return the exception as part of the Result of the task, or just let the IOException bubble up?
|
To quickly react to your last points involving the concurrent solver: When your specification semantically works with the concurrent solver, and there is no type-directed transformation, I think it is fine to use the traditional solver for now, and switch to the concurrent solver later. I'll add a TODO to that PR. |
Breaks printing ints!!!
Called when an object is no longer in accessible memory
Compiling to LLVM in Spoofax 3 is not yet functional
* Records were handled incorrectly by gc * Function type was not converted to closure type everywhere just yet
WIP better closure typing
In some cases free variables might be added after processing a function
|
Exactly 11 months ago, I posted a comment about enabling the concurrent solver. I don't remember the discussion fully anymore, but the referred PR has landed, so technically, enabling the concurrent solver might be possible. |
This PR contains the following changes:
tim/dynamix/dynamix_runtimespoofax2 projects use source imports to depend on each other.Notes/issues that would be nice for you to take a look at:
ReadDynamixSpecTaskDefis possibly a bit over the top and could be changed to just passing aResourcePath, and having the reading be done in the dynamix runtime instead, but I wasn't entirely sure if that would work properly with PIE.createDynamixRuntimeConfiginExecuteDynamixSpecificationTaskDef.java.mustache. This works, but there might be a more idiomatic approach?dynamix_runtime"language" is currently in the metalang folder, but it might be more appropriate to put it elsewhere (next to rv32im?).timstratego interpreter is currently bundled with the metalang (and not split intotim_runtimeor something similar). Currently done because it works and as mentioned this shouldn't be the long-term solution for running tim files.ESVfiles don't seem to work. Try uncommenting thetim/colorsimport in the dynamix Main.esv.0.1.0-SNAPSHOT. Is that fine?Other notes/TODOs: