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.

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

Wrong line delimiter when enclosing multi-line strings with double quotes. #2293

Closed
miklossy opened this issue Jul 7, 2019 · 14 comments · Fixed by #3004
Closed

Wrong line delimiter when enclosing multi-line strings with double quotes. #2293

miklossy opened this issue Jul 7, 2019 · 14 comments · Fixed by #3004
Milestone

Comments

@miklossy
Copy link
Contributor

miklossy commented Jul 7, 2019

After checking out a fresh Xtend workspace on windows, there are several java files in xtend-gen folder marked as dirty:
screenshot1

screenshot2
screenshot3
screenshot4
screenshot5

As far as I can see, the reason for the change is that multi line Strings enclosed by double quotes are translated differently on windows and on mac.

Hint: by multi line String enclosed by ''' the newLine() method is used in the generated *.java file. I suggest to also use this approach for multi-line Strings enclosed by ".

@miklossy
Copy link
Contributor Author

miklossy commented Jul 7, 2019

Small example to reproduce:
The xtend code

package foo

class Foo {
	val a = "
		line1
		line2
	"
	
	val b = '''
		line1
		line2
	'''
}

is compiled to the following java code:

package foo;

import org.eclipse.xtend2.lib.StringConcatenation;
import org.eclipse.xtext.xbase.lib.Functions.Function0;

@SuppressWarnings("all")
public class Foo {
  private final String a = "\r\n\t\tline1\r\n\t\tline2\r\n\t";
  
  private final String b = new Function0<String>() {
    @Override
    public String apply() {
      StringConcatenation _builder = new StringConcatenation();
      _builder.append("line1");
      _builder.newLine();
      _builder.append("line2");
      _builder.newLine();
      return _builder.toString();
    }
  }.apply();
}

screenshot6

@miklossy
Copy link
Contributor Author

miklossy commented Jul 7, 2019

For RichStrings, the XtendCompiler._toJavaExpression(RichString richString, ITreeAppendable b) method is called, for String Literals the XtendCompiler._toJavaExpression(XStringLiteral expr, ITreeAppendable b) method is called. I think we should make a difference if the XStringLiteral is a multi-line string and handle such cases similar to RichStrings.

cc @cdietrich , @szarnekow @ArneDeutsch What do you think about this idea?

@szarnekow
Copy link
Contributor

Instead of imposing runtime semantics, I’d rather align this with Java’s multiline strings and always use unix line delimiters.

@miklossy miklossy added this to the Release_2.21 milestone Nov 24, 2019
@ArneDeutsch
Copy link
Contributor

I had a look about this. At first I thought one could simply fix XtendCompiler#_toJavaStatement(RichString richString, ITreeAppendable b, boolean isReferenced). replacing line 412 (b.append("();");) with b.append("(\"\\n\");"); should do the trick. But it hasn't resolved the issues. I dig deeper. There is post processing logic that replaces the line endings with what is configured for the project.

It seems for me locally I could fix the line ending issues by:

  1. Update to latest git (from 2.12 to 2.24.1)
  2. Install it such that linux line endings are used (autocrlf=false)
  3. Renormalize line breaks for all repositories (disable autocrlf for all repositiories and call git add . --renormalize, afterwards replace all changes with latest from head)

At least inside eclipse context, not sure about outside this scope, Xtext/Xtend seems to work right. At least if environment is configured properly. Perhaps this is just an issue to preconfigure our distro appropriatly and document all this in better way. I will keep my installation as is for now and experiment with it.

@szarnekow
Copy link
Contributor

Are there any settings among the ones you described, that we can automatically apply with Oomph when cloning the repos?

@ArneDeutsch
Copy link
Contributor

I have no experience in configuration of Oomph, but I will have a look. Probably @kthoms can help?

@ArneDeutsch
Copy link
Contributor

The autocrlf setting can be applied by creating a .gitattributes file per repository.

https://help.github.com/en/github/using-git/configuring-git-to-handle-line-endings

If we do so and set text eol=lf everyone should get unix line endings. I would propose to do so. But might have side effects for existing users. I can imagine that we have to renormalize the line endings for local existing repositories (at least windows users).

ArneDeutsch referenced this issue in eclipse/xtext-core Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-eclipse Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-extras Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-lib Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-maven Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-umbrella Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-web Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
ArneDeutsch referenced this issue in eclipse/xtext-xtend Dec 18, 2019
Avoids dirty flags on checkout on windows machines.

Signed-off-by: Arne Deutsch <Arne.Deutsch@itemis.de>
@miklossy
Copy link
Contributor Author

miklossy commented Jan 8, 2020

The xtext-extras repository is also affected, since the org.eclipse.xtext.xbase.tests makes heavy use of multline strings enclosed by "
image

@miklossy
Copy link
Contributor Author

miklossy commented Jan 8, 2020

Instead of imposing runtime semantics, I’d rather align this with Java’s multiline strings and always use unix line delimiters.

This could be achieved by converting the line delimiters to Unix style in the LiteralsCompiler.
screenshot
However, this would affect all Xbase languages, not only Xtend.

@LorenzoBettini
Copy link
Contributor

Instead of imposing runtime semantics, I’d rather align this with Java’s multiline strings and always use unix line delimiters.

@szarnekow, I had also suggested this in another issue I cannot find right now, which may have been lost during the transition to the mono-repo.

I had proposed something similar to #2293 (comment)

I seem to remember that there's an earlier place where the parsed string can be "sanitized" by simply removing \r.

@szarnekow
Copy link
Contributor

szarnekow commented Apr 22, 2024

If the compiler always emits Unix linebreaks, the model should never contain Windows line endings, either. Consequently, the value converter would be the right service to sanitize the strings.

@LorenzoBettini
Copy link
Contributor

@szarnekow would this org.eclipse.xtext.conversion.impl.STRINGValueConverter.Implementation.convertFromJavaString(String, boolean, int, StringBuilder) be the right place to remove Windows EOLs? Or is it too much?

@szarnekow
Copy link
Contributor

I'd use a subtype specifically used for Xbase string literal values.

@LorenzoBettini
Copy link
Contributor

@szarnekow OK, thanks for the suggestion!

I've updated the PR #3004

If I understood correctly, 8ea116b#diff-71a02996356cc4a288de10e1aa3233fdc96cbdcda67302af2d119480c4bc4960R259 this should do the trick; locally the tests are still green.

This way, neither the LiteralsCompiler nor the XbaseInterpreter need adjustments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants