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

Fix Slice.literal for multiple calls with identical signature #15009

Merged

Conversation

HertzDevil
Copy link
Contributor

Fixes #14572.

The usual type inference mechanism for method calls calculates the return type exactly once for each combination of call signature. Therefore, if two slice literals have the same element type, arity, and argument types, only the first one gets seen by Crystal::MainVisitor#visit(Crystal::Primitive), thus subsequent literals reach the codegen phase without being expanded. This PR makes the expansion bypass the usual mechanism and happen much earlier, right after checking for .new of lib types.

A consequence is that Slice is now a built-in type and the Slice.literal declaration is no longer necessary; the @[Primitive(:slice_literal)] is a placeholder and does not drive the actual expansion.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:lang labels Sep 18, 2024
@straight-shoota
Copy link
Member

straight-shoota commented Sep 18, 2024

I don't like going from explicit @[Primitive] to an implicit compiler integration. If that's the only way to get this working, I suppose we'll have to do (and maybe rewrite everything later™). But maybe we can find a better way?

This primitive basically expands the call into a different call (and an implicit const declaration as side effect). This seems more similar to a macro than a def. Perhaps we should think about this as a macro primitive instead?
I don't think there's any precedence for that. But I figure it shouldn't be that difficult to implement.
Biggest hurdle is probably that macros are generally only defined on the uninstantiated generic type (Slice), not the generic instance type (Slice(UInt8)) (I think there should be an existing issue about this, but I can't find it). So that's exactly opposite to how Slice.literal works.
We'd have to fix that. At least for the specific case of @[Primitive(:slice_literal)].

@HertzDevil
Copy link
Contributor Author

.new for lib types is a precedent, and there are none for macro primitives. Personally I would prefer not justifying the latter via this experimental feature and I do not think that would look better than the PR in its current state.

Besides, if we eventually standardize a distinct syntactic form for those literals, there has to be some extra compiler integration anyway.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure happy with this change, as commented above. But it's good as a bug fix and hopefully will be a temporary solution.

@straight-shoota straight-shoota added this to the 1.14.0 milestone Sep 24, 2024
@straight-shoota straight-shoota merged commit cde543a into crystal-lang:master Sep 25, 2024
65 checks passed
@HertzDevil HertzDevil deleted the bug/slice-literal-recalculate branch September 29, 2024 20:42
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:lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: unhandled primitive in codegen: slice_literal
2 participants