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

Remove unnecessary file check for CLI arguments #13853

Merged

Conversation

straight-shoota
Copy link
Member

Resolves #13849

This patch momentarily degrades the error UX because read errors do not mention which file they appear on. That should improve with #13852 though, so I'm not taking any extra measures here.

@HertzDevil
Copy link
Contributor

What happens if there is a compilation error and the compiler tries to read the source code again? (This isn't exclusive to those files of course, it more or less broke eval and --stdin-filename already)

@straight-shoota
Copy link
Member Author

Well, then it may not be able to read the source code. But that's not a big deal. It's the same when eval reads from stdin.
I suppose we could consider improvements to that (such as keeping non-file sources in memory - or maybe just all source?).
But I don't think it's really important. These are likely used for rather trivial entrypoint code where source reference shouldn't matter much. It shouldn't hold back the possibility to use these other file types as input.

@HertzDevil
Copy link
Contributor

HertzDevil commented Sep 28, 2023

We do keep them in memory somewhere, the compiler just doesn't use them yet. My main concern here is whether a blocking read is possible simply from showing a compilation error, making it indistinguishable from an infinite loop

@straight-shoota
Copy link
Member Author

No, that can't happen. Every time the compiler reloads a raw source, there's an explicit File.file? check.

@straight-shoota straight-shoota added this to the 1.11.0 milestone Sep 28, 2023
@straight-shoota straight-shoota merged commit 51fb08f into crystal-lang:master Oct 16, 2023
@straight-shoota straight-shoota deleted the fix/compiler-file-check branch October 16, 2023 17:18
Epikest pushed a commit to Epikest/crystal that referenced this pull request Oct 17, 2023
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:cli and removed kind:feature topic:compiler labels Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler cannot read source code from pipe or character device
2 participants