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

Windows line endings break escaped newlines in strings #5831

Closed
obskyr opened this issue Mar 16, 2018 · 30 comments · Fixed by #11810
Closed

Windows line endings break escaped newlines in strings #5831

obskyr opened this issue Mar 16, 2018 · 30 comments · Fixed by #11810

Comments

@obskyr
Copy link

obskyr commented Mar 16, 2018

The following does not work as expected if your file's line endings are \r\n:

text = "This will have \
    newlines and \
    leading whitespace \
    if your line endings are \\r\\n."

Perhaps Crystal wants to support CRLF as a regular newline, perhaps it doesn't - whatever the case, I figured I'd report it.

@straight-shoota
Copy link
Member

IMHO Crystal shouldn't support CRLF in source files at all.

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

Any particular reason? Just to deliberately make future chances of Windows compatibility more difficult, or...? I don't like CRLF either, but that's no reason to deliberately handle them incorrectly.

@straight-shoota
Copy link
Member

Because it's completely unnecessary and avoids annoying line ending issues.
This is not about being correct, it's just a question if Crystal should support if (and if yes, to full extend) or not. If it is decided that Crystal source code only allows LF, then that's deliberately correct.
Editors on Windows are equally capable of handling LF as editors on Unix-likes can handle CRLF. There is absolutely no reason to assume CRLF would be required to provide some sort of Windows compatibility.
Having only one accepted line ending makes life easier for everyone and I don't think there is a compelling reason for CRLF support.

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

Having only one accepted line ending makes life easier for everyone

Well, except for everyone developing on Windows.

Only allowing LF will, for example, require lines like *.cr text eol=lf in .gitattributes, as files will otherwise be checked out with CRLF on Windows. In addition, while editors can generally handle LF even on Windows, they generally default to CRLF when creating new files, etc.

Personally, I think Crystal should aim to be friendly for the developers using it, as opposed to just the developers developing the language itself.

@ysbaddaden
Copy link
Contributor

The issue isn't LF vs CRLF, but that we shouldn't support escaping line breaks with \ as asterite reported numerous times.

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

Oh, that's on its way out for some reason? In the string literals section of the syntax docs, it's quite clearly documented, and I'd say it's a fairly welcome feature. Guess that might not be the case soon, though?

@straight-shoota
Copy link
Member

straight-shoota commented Mar 16, 2018

@obskyr I'm working on Windows as my primary development platform. None of my source code files uses CRLF (unless explicitly required) and this really keeps away pain.

This is way easier to achieve than you think. Put autocrlf = false in your global git config and no file will be checked out with CRLF. Editors can be configured (if not, go looking for a real one). Of course, I would always recommend using LF as default line ending. There is also .editorconfig to have interoperable settings per repo. Crystal's default .editorconfig already includes end_of_line = lf, so if your editor supports editorconfig, it shouldn't even save a file using CRLF.

To me, developer friendly means to make life easier avoiding unnecessary choices and less things to worry about.

@ysbaddaden IMHO it's a different issue. It shows that the implementation has to keep track of different line endings in many places (not just regarding line continuation). This would be easier if only LF is supported.

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

@straight-shoota Requiring Windows users to either use EditorConfig or change their global git config is most definitely not "developer friendly".

@straight-shoota
Copy link
Member

I think it is, because it's a setting you have to set once and can forget it. Otherwise you will inevitably run into issues with non-matching line endings, mistakenly commit wrong line endings etc.
There is generally no reason to use CRLF on Windows at all.

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

Otherwise you will inevitably run into issues with non-matching line endings, mistakenly commit wrong line endings etc.

You won't if Crystal supports them properly. Instead of having it work only for people who have manually set global settings, why not have it work for everyone?

@straight-shoota
Copy link
Member

Sigh... is this so hard to understand? Whether Crystal supports CRLF or not has nothing to do with the issues I mentioned.
My argument is about having a sane codebase that is easily compatibly with any platform. This is completely independent of which programming language you use. Most languages support both CRLF and LF, but many code conventions recommend using LF only.

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

Hey, there's no need to be condescending.

So, you're thinking Crystal should just error out if it encounters a CRLF linebreak while parsing?

@straight-shoota
Copy link
Member

straight-shoota commented Mar 16, 2018

Essentially, yes. This is obviously not entirely thought through, just an idea to contemplate. Don't know what core members think.

CR-only newlines for example are not supported and result in an syntax error: expected '\n' after '\r'. What if CR was never valid whitespace token in Crystal source code even if followed by LF?

@obskyr
Copy link
Author

obskyr commented Mar 16, 2018

I really am not at all into the idea of having to add *.cr text eol=lf to every Crystal project I ever do with git. Even if I were to set my global git config, I can't really tell everyone who might clone the repo to do so.

@RX14
Copy link
Contributor

RX14 commented Mar 16, 2018

We should either support all like endings in the compiler or error out if the line endings are wrong.

There's an actionable item either way.

@faustinoaq
Copy link
Contributor

We should either support all like endings in the compiler or error out if the line endings are wrong.

What do Rust, Go, Nim in this case?

@RX14
Copy link
Contributor

RX14 commented Mar 17, 2018

Probably support them, and so should crystal.

@faustinoaq
Copy link
Contributor

Interesting, I had a similar issue some time ago

End of line sequence CRLF do not allow to detect symbols, use LF instead.

https://github.com/crystal-lang-tools/vscode-crystal-lang/wiki/Known-Issues

@wooster0
Copy link
Contributor

Yes! I have the same issue! I'm also developing on Windows and I can't create these Strings.

@obskyr
Copy link
Author

obskyr commented Mar 17, 2018

Another related consideration - with \r\n newlines, consider a typical multiline string:

"A multiline
string"

Currently, it evaluates to "A multiline\r\nstring". Regardless of the line endings in your file, newlines in multiline strings should evaluate to \n.

@ghost
Copy link

ghost commented Mar 18, 2018

On Nim:

https://nim-lang.org/docs/manual.html

Encoding

All Nim source files are in the UTF-8 encoding (or its ASCII subset). Other encodings are not supported. Any of the standard platform line termination sequences can be used - the Unix form using ASCII LF (linefeed), the Windows form using the ASCII sequence CR LF (return followed by linefeed), or the old Macintosh form using the ASCII CR (return) character. All of these forms can be used equally, regardless of platform.


I suppose they all support CR LF so it may be useful for crystal to support the other variants it as-is (also, the docu mentions it, so users may assume that \r works fine; if it does not work then I would assume users to expect that the documentation reflects this as well),

Aside from this, I personally agree with straight-shoota (I myself never use \r, ever) but I think if "everyone else treats CR LF like newlines", such as the mentioned programming languages above for windows, then perhaps crystal should do so as well. You could also say that this is one small step for crystal to regard windows as a main platform ... :)

@RX14
Copy link
Contributor

RX14 commented Mar 18, 2018

\r, \r\n and \n should just be converted to \n as early as practical in the lexer.

@go2null
Copy link

go2null commented Apr 28, 2018

Using CRLF causes major pains with codebases with devs on Mac, Linux and Windows. Someone inevitiably messes things up.

@alex-lairan
Copy link

I have an issue with CRLF.

  • Installed Crystal 1.3.0 Windows
  • Create a test app
  • Add ameba to development_dependencies
  • shards install
shards build
Dependencies are satisfied
Building: ameba
Error target ameba failed to compile:
In src\ameba\cli\cmd.cr:173:13

 173 | puts \
             ^
Error: unknown token: '\r'

make: *** [Makefile:8: bin/ameba] Error 1

Windows will inevitably create a file with CRLF somewhere, this is a reason why Crystal has to handle CRLF.

I continue my deep dive of Crystal with Windows 🌊🌊🌊

@alex-lairan
Copy link

alex-lairan commented Jan 8, 2022

Using CRLF causes major pains with codebases with devs on Mac, Linux and Windows. Someone inevitiably messes things up.

Maybe crystal format could handle it.

IMHO the language should handle the case anyway and encourage the LF way 🤘

@HertzDevil
Copy link
Contributor

The CRLF will probably go away if you define core.autocrlf=false at an appropriate scope, since shards relies on Git for many of its operations.

My opinion is the formatter should be able to normalize Crystal source files to LF, so that things continue to work even without EditorConfig; the formatter alone should be able to produce the same effects as the default .editorconfig generated by crystal init, which contains end_of_line = lf. To do this it follows the lexer would have to support the alternative line endings. This may be alleviated by actually supporting the binary mode flag to File.open, or rather, the lack of it (#11586 (comment)); files could be opened in text mode and written in binary mode by the formatter.

@Sija
Copy link
Contributor

Sija commented Jan 8, 2022

IMHO the language should handle the case anyway and encourage the LF way 🤘

Totally, LF is the way, yet I cannot imagine a situation in which you cannot run code with CRLF line endings out-of-the-box.

@straight-shoota
Copy link
Member

I suppose we can support CRLF in the parser and have the formatter change them into LF.

@asterite
Copy link
Member

asterite commented Jan 8, 2022

I don't understand what the issue is with supporting both CRLF and LF. Making the formatter change CRLF to LF might break someone's editor, maybe. Not sure! I think fixing this is trivial. The formatter enhancement is a separate topic. I'll send a PR later this week when ai have time.

@Tamnac
Copy link
Contributor

Tamnac commented Feb 8, 2022

Hi, regardless of what you end up choosing, a more explicit error message would go a long way. This caused me a bunch of trouble.

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

Successfully merging a pull request may close this issue.