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

Issue with Crystal Nightly #372

Closed
mamantoha opened this issue May 9, 2023 · 12 comments · Fixed by #373
Closed

Issue with Crystal Nightly #372

mamantoha opened this issue May 9, 2023 · 12 comments · Fixed by #373

Comments

@mamantoha
Copy link
Contributor

How to reproduce:

crystal spec

Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'macro_140716797307408'

Code in /var/lib/snapd/snap/crystal/1627/share/crystal/src/yaml/serialization.cr:188:7

 188 | {% begin %}
       ^
Called macro defined in /var/lib/snapd/snap/crystal/1627/share/crystal/src/yaml/serialization.cr:188:7

 188 | {% begin %}

Which expanded to:

 > 69 |                   
 > 70 | 
 > 71 |                   __temp_744 =
                          ^
Error: type must be Ameba::Severity, not (Ameba::Severity | Nil)

Probably related to recent changes in Crystal - crystal-lang/crystal#13433

@Sija
Copy link
Member

Sija commented May 9, 2023

Most likely a duplicate of #366

@mamantoha Could you please check if the version from master works for you?

@straight-shoota
Copy link
Contributor

Yes, this is literally the same error message as #371. It should not only appear with Crystal nightly but since 1.8.0.

@Sija
Copy link
Member

Sija commented May 9, 2023

@straight-shoota There's no error for Crystal 1.8.2.

@Sija
Copy link
Member

Sija commented May 9, 2023

@straight-shoota Also, the #366 fixed the type in the specs, and not in the main application code.

@mamantoha
Copy link
Contributor Author

@Sija @straight-shoota I checked this on master.

No issue on Crystal 1.8.1

Crystal 1.8.1 [a59a3dbd7] (2023-04-20)

LLVM: 15.0.7
Default target: x86_64-unknown-linux-gnu

Additionally, I updated the Crystal snap from the edge channel and the error no longer appears.

crystal -v
Crystal 1.8.2 [7aa5cdd86] (2023-05-09)

LLVM: 15.0.7
Default target: x86_64-unknown-linux-gnu

@Sija
Copy link
Member

Sija commented May 9, 2023

Interesting... perhaps an edge-case regression?

@mamantoha
Copy link
Contributor Author

Perhaps. If the issue reappears, I will reopen the ticket.

@Sija
Copy link
Member

Sija commented May 9, 2023

@straight-shoota The issue still persists with Crystal 1.9.0-dev, so sth seems broken there...

@mamantoha mamantoha reopened this May 9, 2023
@mamantoha
Copy link
Contributor Author

I have confirmed the issue on the Crystal master branch by testing it with CRYSTAL_PATH. Setting the CRYSTAL_PATH to /path/to/crystal/src:lib and running crystal spec resulted in the same error as described in the ticket.

@straight-shoota
Copy link
Contributor

straight-shoota commented May 9, 2023

The base issue is the same as #366 though: SeverityYamlConverter.from_yaml returns Severity? and that value is assigned to a variable of type Severity.
This worked before only because the Nil type was silently ignored (an actual nil value would've caused a runtime exception).

This error now appears since crystal-lang/crystal#13433 regardless of the fix in #366 because it affects a different location.
For some reason this code path was previously not touched by the changes to JSON::Serializable in 1.8.x.

To resolve this, the return type of SeverityYamlConverter.from_yaml needs to be assignable to the instance variable. It's probably best to drop the Nil type and raise directly in the converter.

@Sija
Copy link
Member

Sija commented May 9, 2023

@mamantoha Could you try ameba's master branch?

@zw963
Copy link
Contributor

zw963 commented Jul 30, 2023

Test on 1.5.0, it works, thank you!

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