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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: unhandled primitive in codegen: slice_literal
2 participants