Fixing issue 916 on URI path interpolation by jurgenvinju · Pull Request #1802 · usethesource/rascal

jurgenvinju · 2023-06-01T12:43:18Z

removes deprecated calls and fixes modules names in tests
fixes loc splicing does not build URIs the right way (e.g. spaces end up being not encoded when the loc is printed) #916
added tests for loc splicing does not build URIs the right way (e.g. spaces end up being not encoded when the loc is printed) #916

jurgenvinju · 2023-06-01T12:45:18Z

This one was open since 2015, but now more and more people get confused by it. It's probably because of the increased usage of clair which has path interpolation in its source code. Seems like a good time as any to fix it. At least we have some tests for it now.

DavyLandman

The RascalJUnitTestRunner changes confuse me a bit.

DavyLandman · 2023-06-01T12:46:41Z

src/org/rascalmpl/semantics/dynamic/PathPart.java

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


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

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.

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.

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

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.

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.

DavyLandman · 2023-06-01T12:47:33Z

src/org/rascalmpl/test/infrastructure/RascalJUnitTestRunner.java

                    // 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())));


is this related to this PR?

No this was an accident

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

that's nice :) okay, well, I'll allow it ;) ;)

DavyLandman · 2023-06-01T12:47:50Z

src/org/rascalmpl/test/infrastructure/RascalJUnitTestRunner.java

        @Override
        public void start(String context, int count) {
-            notifier.fireTestRunStarted(module);
+            // notifier.fireTestRunStarted(module);


are we sure we want to disable these notifications?

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.

DavyLandman · 2023-06-01T12:48:15Z

src/org/rascalmpl/semantics/dynamic/PathTail.java

+			IString path = (IString) ResultFactory.makeResult(TF.stringType(), VF.string(""), __eval)
+				.add(expr).getValue();
+
+			try {


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

DavyLandman · 2023-06-01T12:49:51Z

src/org/rascalmpl/library/lang/rascal/tests/basic/Locations.rsc

-test bool correctTempPathResolverOnWindows() = /\\/ !:= resolveLocation(|tmp:///|).path;
+test bool correctTempPathResolverOnWindows() = /\\/ !:= resolveLocation(|tmp:///|).path;
+
+test bool encodingIsTheSameForPathAdditionAndTemplateInstantation()


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

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.

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

something like that should work on windows right?

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

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

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.

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

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.

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

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

jurgenvinju · 2023-06-01T14:28:59Z

The current solution is broken. Another test for toLocation shows that the current way of interpolating path strings also introduces an extra / even if the interpolation is in the authority part of a URI.

Have to figure out a way to see if we are already in the path part somehow during the interpolation process.
Have to make sure we encode differently in the authority part then in the path part.

jurgenvinju added 3 commits June 1, 2023 14:31

removes deprecated calls and fixes modules names in tests

e10bf78

fixes #916

9c858c0

added tests for #916

1fbda1c

jurgenvinju requested review from DavyLandman and PaulKlint June 1, 2023 12:44

DavyLandman requested changes Jun 1, 2023

View reviewed changes

DavyLandman reviewed Jun 1, 2023

View reviewed changes

added comments

15c42f4

jurgenvinju marked this pull request as draft June 1, 2023 14:27

DavyLandman mentioned this pull request Jun 1, 2023

random test Locations::top2 failed during another test run #1803

Open

Merge branch 'main' into issue916

bdcb5a9

Comments

Conversation

jurgenvinju commented Jun 1, 2023

Uh oh!

jurgenvinju commented Jun 1, 2023

Uh oh!

DavyLandman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jurgenvinju commented Jun 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants