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

Crystal 1.11.0 cannot build older Crystal stdlib #14194

Closed
straight-shoota opened this issue Jan 9, 2024 · 6 comments
Closed

Crystal 1.11.0 cannot build older Crystal stdlib #14194

straight-shoota opened this issue Jan 9, 2024 · 6 comments
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:compiler:semantic
Milestone

Comments

@straight-shoota
Copy link
Member

The 1.11.0 compiler is unable to build a program with an older version of stdlib. The reason for this is the introduction of ReferenceStorage(T) (#14106). The compiler knows this type and defines it, yet it does not fully implement it. Current stlib does take care of this, but older versions do not, leaving the implementation missing.

This is the error message:

Error: abstract `def Object#hash(hasher)` must be implemented by ReferenceStorage(T)
@straight-shoota straight-shoota added kind:bug topic:compiler:semantic kind:regression Something that used to correctly work but no longer works labels Jan 9, 2024
@straight-shoota
Copy link
Member Author

ReferenceStorage is experimental anyways, so the easiest solution would be to revert that commit.

Alternatively, we could implement a backwards-compatible definition. The compiler could only use this type if it's already defined in the program, otherwise skip it. Or we define the proper inheritance in the compiler, I suppose we just need to explicitly declare that it inherits struct or something.

@straight-shoota straight-shoota added this to the 1.11.1 milestone Jan 9, 2024
@HertzDevil
Copy link
Contributor

HertzDevil commented Jan 10, 2024

ReferenceStorage does not inherit from Struct, which is why the error shows up in the first place.

The solution would probably be having the first definition in code create the underlying class, rather than making it built-in. The definition in code would then be annotated, taking advantage of the fact Crystal never disallows @[Primitive] on class definitions:

@[Primitive(:reference_storage_type)]
struct ReferenceStorage(T) < Value
end

Additionally, this may provide more freedom in renaming the type since it isn't hardcoded across both the compiler and the standard library.

@straight-shoota
Copy link
Member Author

#14204 looks like a great solution 🚀

I feel this might be a bit much for a patch release though. So perhaps we better just revert #14106 for now and merge the new implementation later (i.e. for 0.12.0)?
ReferenceStorage is completely new and experimental, so it wouldn't be a big issue to postpone it. It's a bit of a bummer that experimenting wouldn't be possible then with 1.11 and it will require a newer compiler build. But nightlies should be fine for experimentation, so I don't think it's a big loss.

@beta-ziliani
Copy link
Member

I agree with @straight-shoota that it might be convenient to revert and take these three months to work on it

@zw963
Copy link
Contributor

zw963 commented Jan 10, 2024

I meet basically same issue, when build 1.11.0, i meet #14195 and interpreter not work, when i want to build 1.10.1 use new compiler, it failed.

@straight-shoota
Copy link
Member Author

Resolved by #14207

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:compiler:semantic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants