EclipseSourceFolder.getPath should blindly add a trailing slash

[LorenzoBettini]

The current implementation is

    @Override
    public URI getPath() {
        return URI.createPlatformResourceURI("/" + project.getName() + "/" + name + "/", true);
    }

So, a trailing slash is always added. This usually is harmless, but when org.eclipse.xtext.workspace.ISourceFolder.contains(URI) kicks in:

    default boolean contains(URI uri) {
        URI path = getPath();
        return UriUtil.isPrefixOf(path, uri);
    }

This is heavily used by trace file generation to create the source file relative path. In particular, from org.eclipse.xtext.ui.workspace.EclipseProjectConfig.findSourceFolderContaining(URI):

    @Override
    public ISourceFolder findSourceFolderContaining(URI member) {
        Optional<? extends ISourceFolder> srcFolder = getSourceFolders().stream().filter(folder -> folder.contains(member)).findFirst();
        return srcFolder.isPresent() ? srcFolder.get() : null;

    }

By the way, the source code should be split into several lines to make debugging easier, and the test for isPresent is an anti-pattern (one should do orElse(null)).

When the .classpath has source folders with a trailing slash, the contains method returns false, and the relative path in the trace file is wrong, breaking debugging of an Xbase DSL and the jump to the original DSL member from a Java file. (It was hard to track down the problem).

I seem to understand that trailing slashes in .classpath are admissible, and they are alos automatically added when updating a Maven/Tycho project because, typically, in build.properties, source folders have trailing slashes.

Maybe, EclipseSourceFolder.getPath should be implemented similar to FileSourceFolder concerning the trailing slash:

    @Override
    public URI getPath() {
        final URI result = URI.createFileURI(name).resolve(parent.getPath());
        if (result.hasTrailingPathSeparator()) {
            return result;
        } else {
            return result.appendSegment("");
        }
    }

@szarnekow what do you think?

The problem is that I don't understand where this method is tested.

[szarnekow]

@LorenzoBettini Yes, both implementations must be symmetrical and the implementation from FileSourceFolder appears to be guarding against surprising input. I think we can add dedicated unit tests for both implementations. It appears to me that equals and hashCode are also sensitive to the shape of the given name. I'd think equals and hashCode could be based on the path.

[LorenzoBettini]

@szarnekow Indeed in FileSourceFolder toString, equals and hashCode are already based on path, while in EclipseSourceFolder they're based on project and name.

[LorenzoBettini]

@szarnekow, concerning FileSourceFolder, would these tests be OK?

package org.eclipse.xtext.workspace;

import static org.hamcrest.MatcherAssert.*;
import static org.hamcrest.core.StringEndsWith.*;
import static org.junit.jupiter.api.Assertions.*;

import java.io.File;

import org.junit.Test;

/**
 * @author Lorenzo Bettini - Initial contribution and API
 */
public class FileSourceFolderTest {

    @Test
    public void testNoTrailingPathSeparator() {
        FileSourceFolder fileSourceFolder = new FileSourceFolder(
            new FileProjectConfig(new File("lib")), "aname");
        assertThat(fileSourceFolder.toString(), endsWith("aname/)"));
    }

    @Test
    public void testTrailingPathSeparator() {
        FileSourceFolder fileSourceFolder = new FileSourceFolder(
                new FileProjectConfig(new File("lib")), "aname/");
        assertThat(fileSourceFolder.toString(), endsWith("aname/)"));
    }

    @Test
    public void testEqualsHashcode() {
        FileSourceFolder fileSourceFolderWithTrailingSeparator = new FileSourceFolder(
                new FileProjectConfig(new File("lib")), "aname/");
        FileSourceFolder fileSourceFolderWithoutTrailingSeparator = new FileSourceFolder(
                new FileProjectConfig(new File("lib")), "aname");
        assertEquals(fileSourceFolderWithTrailingSeparator,
                fileSourceFolderWithoutTrailingSeparator);
        assertEquals(fileSourceFolderWithTrailingSeparator.hashCode(),
                fileSourceFolderWithoutTrailingSeparator.hashCode());
    }
}

[szarnekow]

@szarnekow, concerning FileSourceFolder, would these tests be OK?

Yes, absolutely.

[LorenzoBettini]

OK, so in the meantime I pushed this test to my branch and then cherry pick it for a PR in xtext-core, then I'll modify EclipseSourceFolder and create a test in xtext-eclipse