Specs are broken on Crystal master #88
Comments
|
This patch seems to fix most of the error. There's still one spec failing, because a different error is raised then the exected one. But it looks like the actual error might be valid as well? --- i/src/habitat.cr
+++ w/src/habitat.cr
@@ -231,11 +231,12 @@ class Habitat
# NOTE: We can't use the macro level `type.resolve.nilable?` here because
# there's a few declaration types that don't respond to it which would make the logic
# more complex. Metaclass, and Proc types are the main, but there may be more.
- {% if decl.type.is_a?(Union) && decl.type.types.map(&.id).includes?(Nil.id) %}
+ {% if Nil <= decl.type.resolve %}Spec outputEDIT: Ah no, apparently this won't work if the path needs to be resolved in the local scope (e.g. |
|
Oh wow. Thanks for reporting this! I wasn't aware of the ecosystem test. That's pretty amazing. I don't have time to look in to this at the moment, but I'll dive in next week. I wonder if I can just say includes Nil or includes ::Nil 🤔 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Specs for habitat are failing on current Crystal master / nightly build. This was discovered in the ecosystem tests (https://github.com/crystal-lang/test-ecosystem/actions/runs/9306105007/job/25614556241).
The responsible code is this:
habitat/src/habitat.cr
Lines 234 to 238 in 5fd8d7b
It compares type membership based
idwhich is based on the name representation instead of the type itself. Names can be ambiguous, i.e. there are multiple names for the same type. Here the problem is that the idNilis different from::Nil.The failure seems to be caused by a change in the compiler which fixes the id of global paths to include the leading
::which had previsouly been missing (crystal-lang/crystal#14490).With
declasfoo : String?, the expressiondecl.type.types.map(&.id)evaluates to[String, Nil]on Crystal 1.12.2. In Crystal master it's[String, ::Nil](which is correct becauseString?is equivalent toString | ::Nil).Simply changing the check to
includes?(::Nil.id)seems logical, but that won't work because::Nilimplicitly resolves to aTypeNode, not aPath. The path returned fromTypeNode#idis always global, but it does not have a::prefix.My initial understanding is that this code shouldn't have worked in the first place because it relied on the erroneous
idgeneration that was fixed in the mentioned issue.We have to analyze the compiler issue a bit more and it might be possible that another compiler change might fix this. So for now I'm just giving a heads up.
And maybe there's a chance to refactor this macro code so it won't break in the future if it was indeed only working because it relied on erroneous behaviour.
Spec output
You can reproduce this with a nightly build of Crystal (for example
docker run --rm -it -v $(pwd):/app -w /app crystallang/crystal:nightly-alpine crystal spec).The text was updated successfully, but these errors were encountered: