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

Jump to bottom

Fixing issue 916 on URI path interpolation #1802

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Fixing issue 916 on URI path interpolation #1802

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e10bf78
removes deprecated calls and fixes modules names in tests
jurgenvinju Jun 1, 2023
9c858c0
fixes #916
jurgenvinju Jun 1, 2023
1fbda1c
added tests for #916
jurgenvinju Jun 1, 2023
15c42f4
added comments
jurgenvinju Jun 1, 2023
bdcb5a9
Merge branch 'main' into issue916
jurgenvinju May 9, 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
9 changes: 8 additions & 1 deletion src/org/rascalmpl/library/lang/rascal/tests/basic/Locations.rsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,4 +518,11 @@ test bool extensionSetSimple()


// we don't want backslashes in windows
test bool correctTempPathResolverOnWindows() = /\\/ !:= resolveLocation(|tmp:///|).path;
test bool correctTempPathResolverOnWindows() = /\\/ !:= resolveLocation(|tmp:///|).path;

test bool encodingIsTheSameForPathAdditionAndTemplateInstantation()
Copy link
Member

Choose a reason for hiding this comment

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

can we also make one that tests the behavior on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no OS dependent code in this part of the implementation. I.e. we do not use new File or anything like that or Path.separator.

Perhaps we should? Debatable. But right now we mimick exactly the behavior of the + semantics which also does not depend on OS specifics because it simply uses: getValueFactory().sourceLocation(scheme, authority, newPath, query, fragment);, via the fieldUpdate code on SourceLocationResult.

Copy link
Member

Choose a reason for hiding this comment

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

x = "c:\x\t.txt";
return |file:///<x>| == |file:///c:/x/t.txt|;

something like that should work on windows right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure that it will not. There is no code in scope that will interpret the \ as a path separator, so I'm pretty sure they will just stay where they are.

Also, in this example you'd have to escape the backslashes: x = c:\\x\\t.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

So I'd expect it will print: |file:///c:%5Cx%5Ct.txt|, just like the + operator does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, confirmed. the + operator and the fixed interpolator both will not interpret windows path separators, even if on windows. It will just be |file:///c:%5Cx%5Ct.txt| where the \ is escaped as %5C per the URI spec.

Copy link
Member

Choose a reason for hiding this comment

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

ah, okay, I must be missing the case that this fix is solving :) I assumed it was trying to fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This current fix solves the case where x = " "; |file:///<x>|; would throw a URISyntaxException because there are spaces in the path. Instead it now behaves like |file:///| + x where the output would be |file:///%20%20%20|

The fix you have in mind requires a RAP. I'm open to the suggestion, but we'd have to carefully consider letting OS-dependent semantics into the Rascal language. The issue I have with that is that code that is run for a certain experiment could behave differently if run on another machine, with the same input data. So that's definitely worth another discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather put OS-dependent path conversion this under a well-documented library function than hide it under language semantics. Something like: loc parseFilePath(str path, OS forOS=currentOS())

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather put OS-dependent path conversion under a well-documented library function than hide it under language semantics. Something like: loc parseFilePath(str path, OS forOS=currentOS())

= |file:///<"some path">/<"with spaces">| == |file:///| + "some path" + "with spaces";

test bool encodingDecodingPaths()
= (|file:///| + x).path == ((/^\// := x) ? x : "/<x>")
when x := "some path right?";
33 changes: 30 additions & 3 deletions src/org/rascalmpl/semantics/dynamic/PathPart.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,21 @@
*******************************************************************************/
package org.rascalmpl.semantics.dynamic;

import java.net.URI;
import java.net.URISyntaxException;

import org.rascalmpl.ast.Expression;
import org.rascalmpl.ast.PathChars;
import org.rascalmpl.ast.PathTail;
import org.rascalmpl.ast.PrePathChars;
import org.rascalmpl.exceptions.RuntimeExceptionFactory;
import org.rascalmpl.interpreter.IEvaluator;
import org.rascalmpl.interpreter.result.Result;
import org.rascalmpl.interpreter.result.ResultFactory;
import org.rascalmpl.uri.URIUtil;
import io.usethesource.vallang.IConstructor;
import io.usethesource.vallang.ISourceLocation;
import io.usethesource.vallang.IString;
import io.usethesource.vallang.IValue;

public abstract class PathPart extends org.rascalmpl.ast.PathPart {
Expand All @@ -37,9 +44,29 @@ public Result<IValue> interpret(IEvaluator<Result<IValue>> __eval) {

Result<IValue> pre = this.getPre().interpret(__eval);
Result<IValue> expr = this.getExpression().interpret(__eval);
Result<IValue> tail = this.getTail().interpret(__eval);

return pre.add(expr).add(tail);
// here we have to encode the string for us in the path part of a uri
// the trick is to use new File which does the encoding for us, in
// a way that conforms to the current OS where we are running. So if someone
// is splicing a windows path here, with the slashes the other way around,
// they are first interpreted as path separators and not encoded as %slash
// However, first we need to map the expression result to string by using
// the `add` semantics of strings:
IString path = (IString) ResultFactory.makeResult(TF.stringType(), VF.string(""), __eval)
.add(expr).getValue();

try {
// reuse our URI encoders here on the unencoded expression part
URI tmp = URIUtil.create("x", "", "/" + path.getValue());
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I was thinking we should deprecate the URI part of URIUtil, can we do this using ISourceLocation?

Copy link
Member Author

Choose a reason for hiding this comment

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

possibly, but we also want to deprecate the URI part of ISourceLocation right? We'd have to think about getRawPath additions to ISourceLocation, and friends.

Copy link
Member Author

Choose a reason for hiding this comment

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

possibly, but we also want to deprecate the URI part of ISourceLocation right? We'd have to think about getRawPath additions to ISourceLocation, and friends.

Copy link
Member

Choose a reason for hiding this comment

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

well, to go from URI to ISourceLocation is fine(ish), but I was thinking that maybe we should not write new URI code when we don't need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how to avoid URI, since I need URI.getRawPath() here.

Copy link
Member

Choose a reason for hiding this comment

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

ah, okay, yeah that seems hackish, but this might be the place this kind of hackish stuff is needed. Can you write a test that checks that it works on windows? (and keeps working in the future/compiler?)

Maybe we need a special flag to say, run this test only on windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this has to do with Windows. This is pure strings and URIs. The word "path" triggers thinking about file paths, but we are really not in that part of the code. This interpolation and the + operator will do the same thing exactly, 100% ,whether on Windows or on Mac or on Linux, for the same input and the same code.

// but get the path out directly anyway, unencoded!
path = VF.string(tmp.getRawPath());

// connect the pre, middle and end pieces
Result<IValue> tail = this.getTail().interpret(__eval);
return pre.add(ResultFactory.makeResult(TF.stringType(), path, __eval)).add(tail);
}
catch (URISyntaxException e) {
throw RuntimeExceptionFactory.malformedURI(path.getValue(), getExpression(), __eval.getStackTrace());
}
}

}
Expand Down
26 changes: 24 additions & 2 deletions src/org/rascalmpl/semantics/dynamic/PathTail.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,21 @@
*******************************************************************************/
package org.rascalmpl.semantics.dynamic;

import java.net.URI;
import java.net.URISyntaxException;

import org.rascalmpl.ast.Expression;
import org.rascalmpl.ast.MidPathChars;
import org.rascalmpl.ast.PostPathChars;
import org.rascalmpl.exceptions.RuntimeExceptionFactory;
import org.rascalmpl.interpreter.IEvaluator;
import org.rascalmpl.interpreter.result.Result;
import org.rascalmpl.interpreter.result.ResultFactory;
import org.rascalmpl.uri.URIUtil;

import io.usethesource.vallang.IConstructor;
import io.usethesource.vallang.ISourceLocation;
import io.usethesource.vallang.IString;
import io.usethesource.vallang.IValue;

public abstract class PathTail extends org.rascalmpl.ast.PathTail {
Expand All @@ -34,9 +42,23 @@ public Mid(ISourceLocation __param1, IConstructor tree, MidPathChars __param2, E
public Result<IValue> interpret(IEvaluator<Result<IValue>> __eval) {
Result<IValue> mid = this.getMid().interpret(__eval);
Result<IValue> expr = this.getExpression().interpret(__eval);
Result<IValue> tail = this.getTail().interpret(__eval);

return mid.add(expr).add(tail);
IString path = (IString) ResultFactory.makeResult(TF.stringType(), VF.string(""), __eval)
.add(expr).getValue();

try {
Copy link
Member

Choose a reason for hiding this comment

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

this part should also get some comments explaining what is happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// reuse our URI encoders here on the unencoded expression part
URI tmp = URIUtil.create("x", "", "/" + path.getValue());
// but get the path out directly anyway, unencoded!
path = VF.string(tmp.getRawPath());

// connect the pre, middle and end pieces
Result<IValue> tail = this.getTail().interpret(__eval);
return mid.add(ResultFactory.makeResult(TF.stringType(), path, __eval)).add(tail);
}
catch (URISyntaxException e) {
throw RuntimeExceptionFactory.malformedURI(path.getValue(), getExpression(), __eval.getStackTrace());
}
}

}
Expand Down
11 changes: 4 additions & 7 deletions src/org/rascalmpl/test/infrastructure/RascalJUnitTestRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,14 +213,14 @@ public Description getDescription() {

// the order of the tests aren't decided by this list so no need to randomly order them.
for (AbstractFunction f : tests) {
modDesc.addChild(Description.createTestDescription(clazz, computeTestName(f.getName(), f.getAst().getLocation())));
modDesc.addChild(Description.createTestDescription(module, computeTestName(f.getName(), f.getAst().getLocation())));
Copy link
Member

Choose a reason for hiding this comment

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

is this related to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

No this was an accident

Copy link
Member Author

Choose a reason for hiding this comment

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

No this was an accident

The RascalJUnitTestRunner changes confuse me a bit.

well, I was confused when this happened, so that makes total sense again. I accidentally merged this change into the PR. I've tested it and there is no effect, positive or negative, to the test execution other than that the right module names are now shown when a test fails in the reports (also in the XML files).

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 nice :) okay, well, I'll allow it ;) ;)

}
}
catch (Throwable e) {
System.err.println("[ERROR] " + e);
desc.addChild(modDesc);

Description testDesc = Description.createTestDescription(clazz, name + "compilation failed", new CompilationFailed() {
Description testDesc = Description.createTestDescription(module, name + "compilation failed", new CompilationFailed() {
@Override
public Class<? extends Annotation> annotationType() {
return getClass();
Expand All @@ -243,7 +243,6 @@ public void run(final RunNotifier notifier) {
if (desc == null) {
desc = getDescription();
}
notifier.fireTestRunStarted(desc);

for (Description mod : desc.getChildren()) {
if (mod.getAnnotations().stream().anyMatch(t -> t instanceof CompilationFailed)) {
Expand All @@ -255,8 +254,6 @@ public void run(final RunNotifier notifier) {
TestEvaluator runner = new TestEvaluator(evaluator, listener);
runner.test(mod.getDisplayName());
}

notifier.fireTestRunFinished(new Result());
}

private final class Listener implements ITestResultListener {
Expand All @@ -283,7 +280,7 @@ private Description getDescription(String name, ISourceLocation loc) {

@Override
public void start(String context, int count) {
notifier.fireTestRunStarted(module);
// notifier.fireTestRunStarted(module);
Copy link
Member

Choose a reason for hiding this comment

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

are we sure we want to disable these notifications?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, these are actually not allowed to be called anymore. They've been kept in for binary backwards compatibility but the javaDoc says "do not invoke" literally.

This comment was marked as duplicate.

Sign in to view

}

@Override
Expand All @@ -306,7 +303,7 @@ public void report(boolean successful, String test, ISourceLocation loc, String

@Override
public void done() {
notifier.fireTestRunFinished(new Result());
// notifier.fireTestRunFinished(new Result());
}
}
}