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

Incomplete message for compiler errors on Windows #12874

Closed
straight-shoota opened this issue Dec 28, 2022 · 4 comments
Closed

Incomplete message for compiler errors on Windows #12874

straight-shoota opened this issue Dec 28, 2022 · 4 comments
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API status:discussion topic:compiler

Comments

@straight-shoota
Copy link
Member

On windows, some compiler errors are not properly reported to the user.

When such an error happens in a required file, the outermost error is a Crystal::CodeError with message "while requiring ...". And that's the only thing that's printed.

Demonstration:

$ echo 'require "bar"' > foo.cr
$ echo '{% `filedoesnotexist` %}' > bar.cr
$ crystal build foo.cr
Error: while requiring "./bar"

On Windows, the error message is Error: while requiring "./bar". Nothing else. That's not a lot to go on.
On Linux it's more elaborate about the actual error cause: Error: error executing command: false, got exit status 1.

I might have seen a similar behaviour on Linux as well at some point. But not sure about that and what exactly caused this.

I think I have isolated the root cause for this in slightly different behaviour of Process.run. This is method is called for the expansion of the system command `filedoesnotexist`, through the backtick method (.`). It ends up calling Process.run("filedoesnotexist", shell: true) which on Linux (and probably all POSIX systems) returns Process::Status with exit code 127, while on Windows it raises FileNotFoundError.
This FileNotFoundError then bubbles up in the compiler but this error type is not expected in the compiler's error handling code (because it's not a Crystal::CodeError). The non-success exit status on the macro system call instead results in a CodeError on Linux, which is then treated differently in the compiler's error handling.

The most obvious action to fix this behaviour is of course to adjust the behaviour of the backtick method (and probably Process.run) to behave the same on Windows as it does on POSIX systems. This is a separate issue discussed in #12873.

However, there are two aspects related to the compiler's error handling:

  1. Errors that appear at a specific code location such as this one (it happens while expanding the macro backtick method) should be wrapped in a Crystal::CodeError to attach the origin location. This should happen even for unexpected exceptions which could appear as good as anywhere.
  2. The compiler's error reporting code should expect unknown/unexpected errors and fully unwrap them, i.e. print the cause(s) as well, not just the top-level error instance.
@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler status:discussion labels Dec 28, 2022
@Blacksmoke16 Blacksmoke16 added the platform:windows Windows support based on the MSVC toolchain / Win32 API label Dec 28, 2022
@HertzDevil
Copy link
Contributor

HertzDevil commented Dec 28, 2022

IIRC this is also what happens if %LLVM_CONFIG% or some OpenSSL configuration is not properly defined when building the compiler itself

Related: #11456

@straight-shoota
Copy link
Member Author

The immediate issue with macro system was fixed in #12893.

The compiler's error reporting now unwraps errors to print nested causes (#12888).

What's left is this:

Errors that appear at a specific code location such as this one (it happens while expanding the macro backtick method) should be wrapped in a Crystal::CodeError to attach the origin location. This should happen even for unexpected exceptions which could appear as good as anywhere.

This is actually a bit more complex than it sounds due to the architecture of the compiler's error hierarchy. It should be integrated in the bigger (currently stalled) efforts to refactor compiler errors.

I think at this point we can consider this issue resolved.

@HertzDevil
Copy link
Contributor

The actual error message might still be empty in one specific case that is rather important when building the compiler itself, see #12985

@straight-shoota
Copy link
Member Author

This is not directly to the original issue here and I think it's sufficient to track that in its own issue.

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. platform:windows Windows support based on the MSVC toolchain / Win32 API status:discussion topic:compiler
Projects
None yet
Development

No branches or pull requests

3 participants