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.

Sign up for GitHub

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

Always load Rascal modules and Java classes of std from the current rascal #2103

Open
wants to merge 5 commits into
base: replace-lib-by-mvn-and-others
Choose a base branch
from
Open

Always load Rascal modules and Java classes of std from the current rascal #2103

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3973469
Always use `std` from the current `rascal` in the REPL
sungshik Dec 16, 2024
be66d4a
Never load Java classes from a non-current `rascal`
sungshik Dec 16, 2024
0cd3f37
Realign messages to convey which `std` is used (and which isn't)
sungshik Dec 17, 2024
5e6b2ce
Add `std` to module path explicitly
sungshik Dec 17, 2024
e0ad0bb
Print path configs with physical location of `std`
sungshik Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,22 @@
<exclude>**/*.class</exclude>
</excludes>
</resource>
<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>
Comment on lines +59 to +74
Copy link
Contributor Author

@sungshik sungshik Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just temporary here for testing. I'll remove it again before merging (assuming we don't really want to package this?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, indeed, let's not.

<resource>
<directory>.</directory>
<filtering>false</filtering>
Expand Down
15 changes: 9 additions & 6 deletions src/org/rascalmpl/interpreter/utils/RascalManifest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,16 @@ public static String getRascalVersionNumber() {
* This looks into the META-INF/MANIFEST.MF file for a Name and Specification-Version
*/
public String getManifestVersionNumber(ISourceLocation project) throws IOException {
Manifest mf = new Manifest(javaManifest(project));
InputStream is = javaManifest(project);
if (is != null) {
sungshik marked this conversation as resolved.
Show resolved Hide resolved
Manifest mf = new Manifest(is);

String bundleName = mf.getMainAttributes().getValue("Name");
if (bundleName != null && bundleName.equals("rascal")) {
String result = mf.getMainAttributes().getValue("Specification-Version");
if (result != null) {
return result;
String bundleName = mf.getMainAttributes().getValue("Name");
if (bundleName != null && bundleName.equals("rascal")) {
String result = mf.getMainAttributes().getValue("Specification-Version");
if (result != null) {
return result;
}
}
}

Expand Down
118 changes: 70 additions & 48 deletions src/org/rascalmpl/library/util/PathConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;
import java.util.function.Predicate;
import java.util.jar.Manifest;

import org.rascalmpl.interpreter.Configuration;
Expand Down Expand Up @@ -470,16 +473,23 @@ public static PathConfig fromSourceProjectRascalManifest(ISourceLocation manifes
IListWriter libsWriter = vf.listWriter();
IListWriter srcsWriter = vf.listWriter();
IListWriter messages = vf.listWriter();

if (!projectName.equals("rascal")) {
// always add the standard library but not for the project named "rascal"
// which contains the source of the standard library
try {
libsWriter.append(resolveCurrentRascalRuntimeJar());
}
catch (IOException e) {
messages.append(Messages.error(e.getMessage(), manifestRoot));
}

// Approach:
// - Rascal modules of `rascal` inside `std` are loaded from the
// "current `rascal`" (as per `resolveCurrentRascalRuntimeJar`).
// - Rascal modules of `rascal` outside `std` are loaded from the
// "current project" of this path config (as per `manifestRoot`).
// - Java classes of `rascal`, regardless of inside/outside `std`, are
// loaded from the current `rascal`.
//
// Thus, Rascal modules and Java classes of `rascal` inside `std` are
// guaranteed to be consistently loaded from the same version.
try {
sungshik marked this conversation as resolved.
Show resolved Hide resolved
srcsWriter.append(URIUtil.rootLocation("std"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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://

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with that.

libsWriter.append(resolveCurrentRascalRuntimeJar());
}
catch (IOException e) {
messages.append(Messages.error(e.getMessage(), manifestRoot));
}

ISourceLocation target = URIUtil.correctLocation("project", projectName, "target/classes");
Expand Down Expand Up @@ -535,15 +545,17 @@ public static PathConfig fromSourceProjectRascalManifest(ISourceLocation manifes
}

if (libProjectName != null) {
if (reg.exists(projectLoc) && dep != rascalProject) {
if (reg.exists(projectLoc)) {
Copy link
Contributor Author

@sungshik sungshik Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sungshik sungshik Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/tutor
  • src/org/rascalmpl/typepal
  • src/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!/|

// 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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

libsWriter.append(childConfig.getBin());
}
break;
case COMPILER:
libsWriter.append(setTargetScheme(projectLoc));
Expand All @@ -556,9 +568,6 @@ public static PathConfig fromSourceProjectRascalManifest(ISourceLocation manifes
// error messages are transitively collected
messages.appendAll(childConfig.getMessages());
}
else if (dep == rascalProject) {
// not adding it again (we added rascal already above)
}
else {
// just a pre-installed dependency in the local maven repository
if (!reg.exists(dep)) {
Expand All @@ -570,7 +579,9 @@ else if (dep == rascalProject) {
libsWriter.append(dep);
break;
case INTERPRETER:
libsWriter.append(dep);
if (dep != rascalProject) { // Never load Java classes from a non-current `rascal`
libsWriter.append(dep);
}
addLibraryToSourcePath(reg, srcsWriter, messages, dep);
break;
default:
Expand All @@ -586,36 +597,42 @@ else if (dep == rascalProject) {
}

try {
if (!projectName.equals("rascal") && rascalProject == null) {
// always add the standard library but not for the project named "rascal"
// which contains the (source of) the standard library, and if we already
// have a dependency on the rascal project we don't add it here either.
var rascalLib = resolveCurrentRascalRuntimeJar();
messages.append(Messages.info("Effective rascal library: " + rascalLib, getPomXmlLocation(manifestRoot)));
libsWriter.append(rascalLib);
}
else if (projectName.equals("rascal")) {
messages.append(Messages.info("detected rascal self-application", getPomXmlLocation(manifestRoot)));
var currentRascal = resolveCurrentRascalRuntimeJar();
sungshik marked this conversation as resolved.
Show resolved Hide resolved
var currentRascalPhysical = reg.logicalToPhysical(currentRascal);
var currentRascalVersion = RascalManifest.getRascalVersionNumber();

// Checks if location `otherRascal` represents the same version of
// the project named "rascal" as location `currentRascal`
Predicate<ISourceLocation> isCurrentRascal = otherRascal -> {
try {
var otherRascalPhysical = reg.logicalToPhysical(otherRascal);
var otherRascalVersion = new RascalManifest().getManifestVersionNumber(otherRascalPhysical);
return Objects.equals(currentRascalPhysical, otherRascalPhysical)
|| Objects.equals(currentRascalVersion, otherRascalVersion);
} catch (IOException e) {
return false;
}
};

Consumer<String> report = s -> messages.append(Messages.info(s, getPomXmlLocation(manifestRoot)));
report.accept("Using Rascal standard library at " + currentRascal + ". Version: " + currentRascalVersion + ".");

// When this path config's project is `rascal`...
if (projectName.equals("rascal") && !isCurrentRascal.test(target)) {
report.accept("Ignoring Rascal standard library at " + target + " (self-application)");
}
else if (rascalProject != null) {
// The Rascal interpreter can not escape its own classpath, whether

// When a dependency of this path config's project is `rascal`...
if (rascalProject != null && !isCurrentRascal.test(rascalProject)) {
report.accept("Ignoring Rascal standard library at " + rascalProject + " (dependency in POM)");

// Note: The Rascal interpreter can not escape its own classpath, whether
// or not we configure a different version in the current project's
// pom.xml or not. So that pom dependency is always ignored!

// We check this also in COMPILED mode, for the sake of consistency,
// but it is not strictly necessary since the compiler can check and compile
// against any standard library on the libs path, even if it's running
// itself against a different rascal runtime and standard library.

RascalManifest rmf = new RascalManifest();
var builtinVersion = RascalManifest.getRascalVersionNumber();
var dependentRascalProject = reg.logicalToPhysical(rascalProject);
var dependentVersion = rmf.getManifestVersionNumber(dependentRascalProject);

if (!builtinVersion.equals(dependentVersion)) {
messages.append(Messages.info("Effective rascal version: " + builtinVersion, getPomXmlLocation(manifestRoot)));
messages.append(Messages.warning("Different rascal dependency is not used: " + dependentVersion, getPomXmlLocation(manifestRoot)));
}
}

ISourceLocation projectLoc = URIUtil.correctLocation("project", projectName, "");
Expand All @@ -635,8 +652,11 @@ else if (rascalProject != null) {
messages.append(Messages.error(e.getMessage(), getRascalMfLocation(manifestRoot)));
}


for (String srcName : manifest.getSourceRoots(manifestRoot)) {
if (refersToStd(projectName, srcName)) {
continue; // Never load Rascal modules of `std` from a non-current `rascal`
}

var srcFolder = URIUtil.getChildLocation(manifestRoot, srcName);

if (!reg.exists(srcFolder) || !reg.isDirectory(srcFolder)) {
Expand All @@ -661,18 +681,16 @@ private static void addLibraryToSourcePath(URIResolverRegistry reg, IListWriter
return;
}


var manifest = new RascalManifest();

// the rascal dependency leads to a dependency on the std:/// location, somewhere _inside_ of the rascal jar
if (manifest.getProjectName(jar).equals("rascal")) {
srcsWriter.append(URIUtil.rootLocation("std"));
return;
}
var projectName = manifest.getProjectName(jar);

boolean foundSrc = false;

for (String src : manifest.getSourceRoots(jar)) {
if (refersToStd(projectName, src)) {
continue; // Never load Rascal modules of `std` from a non-current `rascal`
}

ISourceLocation srcLib = URIUtil.getChildLocation(jar, src);
if (reg.exists(srcLib)) {
srcsWriter.append(srcLib);
Expand All @@ -683,12 +701,16 @@ private static void addLibraryToSourcePath(URIResolverRegistry reg, IListWriter
}
}

if (!foundSrc) {
if (!foundSrc && !projectName.equals("rascal")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this extra condition needed?

rascal has three source roots:

  • src/org/rascalmpl/library
  • test/org/rascalmpl/benchmark
  • test/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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

rascal/src/org/rascalmpl/library/util/PathConfig.java

Line 689 in 5e6b2ce

for (String src : manifest.getSourceRoots(jar)) {

// if we could not find source roots, we default to the jar root
srcsWriter.append(jar);
}
}

private static boolean refersToStd(String projectName, String srcName) {
sungshik marked this conversation as resolved.
Show resolved Hide resolved
return projectName.equals("rascal") && srcName.equals("src/org/rascalmpl/library");
}

private static ISourceLocation setTargetScheme(ISourceLocation projectLoc) {
try {
return URIUtil.changeScheme(projectLoc, "target");
Expand Down