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.15 seems to have symbol Index reserved #15357

Closed
wolfgang371 opened this issue Jan 20, 2025 · 3 comments · Fixed by #15358
Closed

Crystal 1.15 seems to have symbol Index reserved #15357

wolfgang371 opened this issue Jan 20, 2025 · 3 comments · Fixed by #15358
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:runtime
Milestone

Comments

@wolfgang371
Copy link

With Crystal 1.15.0 [7b9e2ef80] (2025-01-09), LLVM: 18.1.6, Default target: x86_64-unknown-linux-gnu running the following ...

alias Index = Int32 # `Index` seems to be reserved now...?

... I get...

In /snap/crystal/2355/share/crystal/src/crystal/event_loop/epoll.cr:6:1
 6 | class Crystal::EventLoop::Epoll < Crystal::EventLoop::Polling
     ^
Error: abstract `def Crystal::EventLoop::Polling#system_add(fd : Int32, index : Index)` must be implemented by Crystal::EventLoop::Epoll

However, there was no issue on prior releases, e.g. 1.14: https://play.crystal-lang.org/#/r/hm4t

@wolfgang371 wolfgang371 added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Jan 20, 2025
@straight-shoota straight-shoota added this to the 1.15.1 milestone Jan 20, 2025
@straight-shoota straight-shoota added topic:stdlib:runtime kind:regression Something that used to correctly work but no longer works labels Jan 20, 2025
@straight-shoota
Copy link
Member

straight-shoota commented Jan 20, 2025

This is a regression from refactoring the internal EventLoop API in #14996.
The new API introduces the mentioned abstract method which has a parameter with type restriction Index. But this type doesn't resolve to anything in stdlib, so the compiler doesn't check for implementations. By introducing a Index type in the top level this path now resolves and forces an implementation of the abstract def which obviously cannot exist.

The fix is simple, adjust the path to point to Arena::Index, the actual target of this restriction.

But I'm wondering if we shouldn't have caught this issue with a compiler error (or warning). (see #15359)

@Blacksmoke16
Copy link
Member

But I'm wondering if we shouldn't have caught this issue with a compiler error (or warning).

Yea I'm surprised this didn't originally error due to an "Undefined constant" or something.

@wolfgang371
Copy link
Author

Index binds with higher prio to the higher level symbol:

alias Index = Int32 # (un-)comment to trigger error

# src/crystal/event_loop/polling/arena.cr
class Crystal::EventLoop::Polling::Arena(T, BLOCK_BYTESIZE)
    # ...
    struct Index
        # ...
    end
end

# src/crystal/event_loop.cr
abstract class Crystal::EventLoop
# ...
end

# src/crystal/event_loop/polling.cr
abstract class Crystal::EventLoop::Polling < Crystal::EventLoop
    # ...
    protected abstract def system_add(fd : Int32, index : Index) : Nil
    # ...
end

github-actions bot pushed a commit that referenced this issue Jan 27, 2025
The abstract method refers to the non existing `Index` type. Weirdly the compiler won't complain until someone defines an `Index` type.

Resolves #15357

(cherry picked from commit 5a245d9)
straight-shoota added a commit that referenced this issue Jan 28, 2025
The abstract method refers to the non existing `Index` type. Weirdly the compiler won't complain until someone defines an `Index` type.

Resolves #15357

(cherry picked from commit 5a245d9)

Co-authored-by: Johannes Müller <[email protected]>
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. kind:regression Something that used to correctly work but no longer works topic:stdlib:runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants