Always load Rascal modules and Java classes of std from the current rascal
#2103
Conversation
| <resource> | ||
| <targetPath>test/org/rascalmpl/benchmark</targetPath> | ||
| <directory>test/org/rascalmpl/benchmark</directory> | ||
| <excludes> | ||
| <exclude>**/*.java</exclude> | ||
| <exclude>**/*.class</exclude> | ||
| </excludes> | ||
| </resource> | ||
| <resource> | ||
| <targetPath>test/org/rascalmpl/test/data</targetPath> | ||
| <directory>test/org/rascalmpl/test/data</directory> | ||
| <excludes> | ||
| <exclude>**/*.java</exclude> | ||
| <exclude>**/*.class</exclude> | ||
| </excludes> | ||
| </resource> |
There was a problem hiding this comment.
This is just temporary here for testing. I'll remove it again before merging (assuming we don't really want to package this?).
| @@ -535,15 +545,17 @@ public static PathConfig fromSourceProjectRascalManifest(ISourceLocation manifes | |||
| } | |||
|
|
|||
| if (libProjectName != null) { | |||
| if (reg.exists(projectLoc) && dep != rascalProject) { | |||
| if (reg.exists(projectLoc)) { | |||
There was a problem hiding this comment.
This conditional was simplified to: either the dependency project is open in the workspace, or it's not. Special case for rascal isn't needed anymore at this level.
There was a problem hiding this comment.
I have some doubts about this. The function is recursive so we might end up following a pom dependency to a rascal project that is open. What exactly happens in this case?
I think what should happen is we indeed only depend on the deployed rascal jar, but additionally to this we should not add the compiler and tutor and typepal to the source path of the current project. So this should require some extra code/conditionals.
There was a problem hiding this comment.
Update: I just re-read your other comment in more detail, and I think it already confirms my understand of what I thought you meant is correct.
Hm, good question! I think you're right. But, I'll try to rephrase your point with some examples to see if I really understand. It's a bit of a lengthy response, but I need to make sure I'm not going to implement the wrong thing 😉.
Suppose, post-merge, the rascal project has the following source folders (names adopted from another comment by Jurgen):
src/org/rascalmpl/tutorsrc/org/rascalmpl/typepalsrc/org/rascalmpl/compiler
Current implementation of this PR
Approach: All non-std source folders of rascal are added to the module path.
If a REPL is created for project a in VS Code, and rascal is open in the workspace, then:
Module paths:
|std:///|
|project://rascal/src/org/rascalmpl/tutor|
|project://rascal/src/org/rascalmpl/typepal|
|project://rascal/src/org/rascalmpl/compiler|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
If a REPL is created for project a in VS Code, and rascal isn't open in the workspace, then:
Module paths:
|std:///|
|mvn://org.rascalmpl--rascal--1.0.0/src/org/rascalmpl/tutor|
|mvn://org.rascalmpl--rascal--1.0.0/src/org/rascalmpl/typepal|
|mvn://org.rascalmpl--rascal--1.0.0/src/org/rascalmpl/compiler|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
Suggested implementation by Jurgen
REPLs created for non-rascal projects
Approach: No non-std source folders of rascal are added to the module path. The rationale is that "normal" users don't need tutor, typepal, and compiler, so it's unnecessary (perhaps even confusing) to make them accessible in the REPL.
If a REPL is created for project a in VS Code, and rascal is open in the workspace, then:
Module paths:
|std:///|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
If a REPL is created for project a in VS Code, and rascal isn't open in the workspace, then:
Module paths:
|std:///|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
I.e., Whether or not rascal is open in the workspace has no impact on the module path of REPLs created for "normal" projects.
REPLs created for rascal
Approach: All non-std source folders of rascal are added to the module path.
Module paths:
|std:///|
|file:///C:/Users/sung-/Desktop/rascal/src/org/rascalmpl/tutor|
|file:///C:/Users/sung-/Desktop/rascal/src/org/rascalmpl/typepal|
|file:///C:/Users/sung-/Desktop/rascal/src/org/rascalmpl/compiler|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-0.12.0-head/assets/jars/rascal-lsp.jar!/|
| @@ -683,12 +701,16 @@ private static void addLibraryToSourcePath(URIResolverRegistry reg, IListWriter | |||
| } | |||
| } | |||
|
|
|||
| if (!foundSrc) { | |||
| if (!foundSrc && !projectName.equals("rascal")) { | |||
There was a problem hiding this comment.
Why is this extra condition needed?
rascal has three source roots:
src/org/rascalmpl/librarytest/org/rascalmpl/benchmarktest/org/rascalmpl/test/data
The test/** roots aren't normally* packaged in the jar, though, so foundSrc becomes false in the previous loop, and the jar root is unnecessarily appended (which may look confusing to the user).
Before this PR, this code was never reached (in the case of rascal) due to an early return (see the removed code in the diff, lines 666-671 in the old file), so it wasn't an issue. After this PR, the early return is gone, so the extra condition is needed here to avoid the append.
(*) I've made changes to pom.xml in this PR to package all source roots, but that's just for testing and probably will be reverted.
There was a problem hiding this comment.
Maybe I'm confused, but they shouldn't be packaged in the jar (as that is what maven defines), but still show up in the search path of the interpreter?
There was a problem hiding this comment.
that's right. We will also be having more new source locations shortly:
- src/org/rascalmpl/tutor
- src/org/rascalmp/typepal
- src/org/rascalmpl/compiler
There was a problem hiding this comment.
Maybe I'm confused, but they shouldn't be packaged in the jar (as that is what maven defines)
Correct, the test/** source roots aren't packaged in the jar.
but still show up in the search path of the interpreter?
Per line 689, the source roots to be included in the path config aren't taken from the jar but from RASCAL.MF:
| // Thus, Rascal modules and Java classes of `rascal` inside `std` are | ||
| // guaranteed to be consistently loaded from the same version. | ||
| try { | ||
| srcsWriter.append(URIUtil.rootLocation("std")); |
There was a problem hiding this comment.
is there a way to not use std ? Or is there no way to figure out what's the rascal.jar that's on the class path?
There was a problem hiding this comment.
It's possible to get the loc of the current rascal jar using PathConfig.getCurrentRascalRuntime(), however it's best to use std:// since the .tpl files for the standard library will be containing those references. So for interpreter stacktraces and debugging information and such to line up with the information that the summary for the LSP uses, we should also use std://
There was a problem hiding this comment.
it's just that for the end user, it's unclear where std came from. it would have been nice to show: hey, it came from.../vscode-extension/.../rascal.jar or whereever. I like the explicitness of that.
There was a problem hiding this comment.
How about we add std:/// to the module path, but we print the physical location in the terminal? Like this:
Module paths:
|std:///| at |file:///C:/Users/sung-/Desktop/rascal3/rascal/target/classes|
|mvn://org.rascalmpl--rascal--0.40.8-REPOSITORY/test/org/rascalmpl/benchmark|
|mvn://org.rascalmpl--rascal--0.40.8-REPOSITORY/test/org/rascalmpl/test/data|
|file:///C:/Users/sung-/Desktop/rascal-which-version/projects/typepal/src|
It's only a few extra code in printInterpreterConfigurationStatus (prototyped the changes already, but not committed yet; let's first (dis)agree).
| // The project we depend on is available in the current workspace. | ||
| // so we configure for using the current state of that project. | ||
| PathConfig childConfig = fromSourceProjectRascalManifest(projectLoc, mode); | ||
|
|
||
| switch (mode) { | ||
| case INTERPRETER: | ||
| srcsWriter.appendAll(childConfig.getSrcs()); | ||
| libsWriter.append(childConfig.getBin()); | ||
| if (dep != rascalProject) { // Never load Java classes from a non-current `rascal` |
There was a problem hiding this comment.
what fails if you do load java classes from a rascal project? as in, the class loader will most likely ignore them? but it has less special cases to this code?
There was a problem hiding this comment.
What happens is that there will be two sources for all runtime classes including vallang and the interpreter and the library components written in Java. Creating new classes might work while adding or changing methods will not. Most likely if the two versions start to become different there will be ClassNotFound and such exceptions thrown by the JavaBridge.
What I find most disturbing is that it becomes much less transparant what the linkage is between Rascal code and Java code to the programmer.
One of the consequences of loading from the latest target folder of the rascal project is failing to bootstrap the tutor due to the above exceptions. Also many tests will fail.
| @@ -683,12 +701,16 @@ private static void addLibraryToSourcePath(URIResolverRegistry reg, IListWriter | |||
| } | |||
| } | |||
|
|
|||
| if (!foundSrc) { | |||
| if (!foundSrc && !projectName.equals("rascal")) { | |||
There was a problem hiding this comment.
Maybe I'm confused, but they shouldn't be packaged in the jar (as that is what maven defines), but still show up in the search path of the interpreter?
I'm not completely clear on what this sentence means. "current rascal" is supposed to mean the released rascal jar that we are currently running, and not the current source code project named "rascal". Is this right? |
There was a problem hiding this comment.
I think this is very nice and will work great for the people developing the compiler code, tutor code and typepal code that will be part of the rascal project.
There is one case that we are not covering yet. If a project depends on a rascal jar via the pom, and the rascal project is open in the workspace, then currently the source code of the compiler, tutor, and typepal, benchmarks and testdata will be added to the interpreter's source path for the current not-rascal project.
I think we need to plug that leak otherwise all rascal projects will be flooded with lots and lots of unnecessary modules. Only |std://| should be on the path for a project that depends on rascal.
Also watch out for double dependencies on |std:///|. That makes the interpreter much slower when modules are imported with a typo or which don't exist yet.
|
With respect to generating documentation for the rascal project, we might need some more thinking. However, that can be solved in the rascal-maven-plugin Java code perhaps? There is it important that the tutor can execute new Java functions while runing So while we are thinking about that, for the current PR please be adviced that |
|
I saw this, is it out of date?
The latter |
|
Also, as the rascal.jar shades all of its dependencies, they should not be taken from the pom.xml. In fact the only thing on the classpath should be the rascal.jar from the current installation. So these dependencies should not be added if we are configuring for the rascal project itself: This would make actual dependencies on jar versions more transparant, and also prevent possible ClassNotFound exceptions etc, by removing duplicates from the classpath. |
| // Thus, Rascal modules and Java classes of `rascal` inside `std` are | ||
| // guaranteed to be consistently loaded from the same version. | ||
| try { | ||
| srcsWriter.append(URIUtil.rootLocation("std")); |
There was a problem hiding this comment.
How about we add std:/// to the module path, but we print the physical location in the terminal? Like this:
Module paths:
|std:///| at |file:///C:/Users/sung-/Desktop/rascal3/rascal/target/classes|
|mvn://org.rascalmpl--rascal--0.40.8-REPOSITORY/test/org/rascalmpl/benchmark|
|mvn://org.rascalmpl--rascal--0.40.8-REPOSITORY/test/org/rascalmpl/test/data|
|file:///C:/Users/sung-/Desktop/rascal-which-version/projects/typepal/src|
It's only a few extra code in printInterpreterConfigurationStatus (prototyped the changes already, but not committed yet; let's first (dis)agree).
| @@ -535,15 +545,17 @@ public static PathConfig fromSourceProjectRascalManifest(ISourceLocation manifes | |||
| } | |||
|
|
|||
| if (libProjectName != null) { | |||
| if (reg.exists(projectLoc) && dep != rascalProject) { | |||
| if (reg.exists(projectLoc)) { | |||
There was a problem hiding this comment.
Update: I just re-read your other comment in more detail, and I think it already confirms my understand of what I thought you meant is correct.
Hm, good question! I think you're right. But, I'll try to rephrase your point with some examples to see if I really understand. It's a bit of a lengthy response, but I need to make sure I'm not going to implement the wrong thing 😉.
Suppose, post-merge, the rascal project has the following source folders (names adopted from another comment by Jurgen):
src/org/rascalmpl/tutorsrc/org/rascalmpl/typepalsrc/org/rascalmpl/compiler
Current implementation of this PR
Approach: All non-std source folders of rascal are added to the module path.
If a REPL is created for project a in VS Code, and rascal is open in the workspace, then:
Module paths:
|std:///|
|project://rascal/src/org/rascalmpl/tutor|
|project://rascal/src/org/rascalmpl/typepal|
|project://rascal/src/org/rascalmpl/compiler|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
If a REPL is created for project a in VS Code, and rascal isn't open in the workspace, then:
Module paths:
|std:///|
|mvn://org.rascalmpl--rascal--1.0.0/src/org/rascalmpl/tutor|
|mvn://org.rascalmpl--rascal--1.0.0/src/org/rascalmpl/typepal|
|mvn://org.rascalmpl--rascal--1.0.0/src/org/rascalmpl/compiler|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
Suggested implementation by Jurgen
REPLs created for non-rascal projects
Approach: No non-std source folders of rascal are added to the module path. The rationale is that "normal" users don't need tutor, typepal, and compiler, so it's unnecessary (perhaps even confusing) to make them accessible in the REPL.
If a REPL is created for project a in VS Code, and rascal is open in the workspace, then:
Module paths:
|std:///|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
If a REPL is created for project a in VS Code, and rascal isn't open in the workspace, then:
Module paths:
|std:///|
|file:///C:/Users/sung-/Desktop/a/src|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-1.0.0/assets/jars/rascal-lsp.jar!/|
I.e., Whether or not rascal is open in the workspace has no impact on the module path of REPLs created for "normal" projects.
REPLs created for rascal
Approach: All non-std source folders of rascal are added to the module path.
Module paths:
|std:///|
|file:///C:/Users/sung-/Desktop/rascal/src/org/rascalmpl/tutor|
|file:///C:/Users/sung-/Desktop/rascal/src/org/rascalmpl/typepal|
|file:///C:/Users/sung-/Desktop/rascal/src/org/rascalmpl/compiler|
|jar+file:///C:/Users/sung-/.vscode/extensions/usethesource.rascalmpl-0.12.0-head/assets/jars/rascal-lsp.jar!/|
| @@ -683,12 +701,16 @@ private static void addLibraryToSourcePath(URIResolverRegistry reg, IListWriter | |||
| } | |||
| } | |||
|
|
|||
| if (!foundSrc) { | |||
| if (!foundSrc && !projectName.equals("rascal")) { | |||
There was a problem hiding this comment.
Maybe I'm confused, but they shouldn't be packaged in the jar (as that is what maven defines)
Correct, the test/** source roots aren't packaged in the jar.
but still show up in the search path of the interpreter?
Per line 689, the source roots to be included in the path config aren't taken from the jar but from RASCAL.MF:
Correct. It's the thing returned by |
|
Thank you, @jurgenvinju, for your detailed comments 🙂. I think I addressed most of them, except three, which I'll summarize here:
I think I'd prefer to address these issues in separate PRs, as they're not entirely related to
|
|
Excellent! I think PR 1 is only possible from a combination of the project here (with the tutor merged here) and the rascal-maben-plugin (to start a nested JVM process with the right Classpath). Note that the current main head does not work anymore in the rascal-maven-plugin context. @rodinaarssen and I are looking to fix that shortly. |
|
I was updating this branch to implement Jurgen's suggestions, but then I realized it can be done with much fewer changes relative to the base branch. So, I prepared a new PR: I'll keep the "attempt 1"-PR open for now, but please have a look at that other PR for the latest version of this work. The first comment of that PR briefly explains the differences between the PRs. |
Overview
This PR implements the following approach for loading Rascal modules and Java classes of
rascal:rascalinsidestdare loaded from the "currentrascal" (as perresolveCurrentRascalRuntimeJar).rascaloutsidestdare loaded from the "current project" of the path config.rascal, regardless of inside/outsidestd, are loaded from the "currentrascal" (as perresolveCurrentRascalRuntimeJar).Thus, after this PR:
✔️ Rascal modules and Java classes of
rascalinsidestdmust be loaded from the same version. This partly fixes Rascal modules and Java classes ofrascalare loaded from different versions rascal-language-servers#538.rascaloutsidestdmay be loaded from different versions. However, currently, there are no Rascal modules ofrascaloutsidestdthat rely on Java classes. Moreover, after the merge oftypepalandrascal-coreintorascal: (1) there are no Rascal modules oftypepalthat rely on Java classes; (2) there are a few Rascal modules ofrascal-corethat rely on Java classes, but the impact seems negligible in the context of this PR.stdof the deployedrascal.jar(part of the VS Code extension), which is put on the class path of the REPL's JVM. Even whenrascalitself is open in the workspace, changes made tostdwill not be loaded in the REPL. To test such changes, a standalone Rascal shell needs to be created (but it doesn't have debugging support).See the comments below for a few more details.
Examples (VS Code)
Create REPL in
rascalCreate REPL in
typepal(withrascalopen in the workspace)Create REPL in
typepal(withoutrascalopen in the workspace)Example (
RascalShell)Create REPL in
rascal(using aRascalShellin the samerascal)Create REPL in
rascal(using aRascalShellin a differentrascal)Create REPL in
typepal