Skip to content

Conversation

@Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Nov 23, 2025

Fixes #10699

I broke out the rescue blocks into one for each side, removing the logic that always just defaulted to true if there was a type lookup error. It'll now properly throw an error if one side has a type that doesn't resolve. E.g. example one in the originally issue. This PR does NOT handle the case where both side are invalid types, but could change this as a follow up if we wanted too.

A type is assumed to be resolvable if it is a free var, global, is a generic type var, or is not a Path. This handles the second example in the original issue. It's still not super strict, as as it stands, the logic allows using a diff free var name, or really even just def foo(x) to satisfy abstract def foo(x : U) : U forall U. This satisfies my use case at least by ensuring at least one T.class overload exists.

EDIT: Moving this to a draft, abstract def get_env(type : Int32) shouldn't be valid given abstract def get_env(type : T.class) : T forall T.

@Blacksmoke16 Blacksmoke16 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Nov 23, 2025
end

abstract def abi_info(atys : Array(LLVM::Type), rty : LLVM::Type, ret_def : Bool, context : Context)
abstract def abi_info(atys : Array(LLVM::Type), rty : LLVM::Type, ret_def : Bool, context : LLVM::Context)
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we're validating these, it errored that it didn't know what Context is.

@Blacksmoke16 Blacksmoke16 marked this pull request as draft November 23, 2025 22:39
@Blacksmoke16 Blacksmoke16 marked this pull request as ready for review November 23, 2025 23:10
@HertzDevil HertzDevil self-requested a review November 24, 2025 02:52
return false
end

return true if restriction.global?
Copy link
Contributor

Choose a reason for hiding this comment

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

At this time all the types have been defined, so I don't really know what this is supposed to do. I would expect to see an error in either abstract def here:

abstract class Foo
  abstract def foo(x : Int32)
  abstract def bar(x : ::A)
end

class Bar < Foo
  def foo(x : ::A); end
  def bar(x : Int32); end
end

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a way to retain current behavior, as without you get a lot of spec failures:

  9) Codegen: is_a? evaluates method on filtered union type 3

       In src/crystal/event_loop/polling.cr:255:21

        255 | def read(socket : ::Socket, slice : Bytes) : Int32
                                ^-------
       Error: undefined constant ::Socket  from src/compiler/crystal/semantic/ast.cr:6:7 in 'raise'
         from src/compiler/crystal/semantic/ast.cr:5:5 in 'raise'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:461:5 in 'report_error'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:335:5 in 'report_undefined_type'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:272:27 in 'check_arg'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:203:29 in 'implements?'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:92:20 in 'check_implemented_in_subtypes'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:79:9 in 'check_single'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:38:7 in 'check_single'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:38:7 in 'check_single'
         from src/compiler/crystal/semantic/abstract_def_checker.cr:38:7 in 'run'
         from src/compiler/crystal/progress_tracker.cr:23:20 in 'top_level_semantic'
         from src/compiler/crystal/semantic.cr:22:23 in 'semantic:cleanup'
         from src/compiler/crystal/compiler.cr:229:16 in 'compile'
         from src/process/shell.cr:14:5 in 'run'
         from spec/compiler/codegen/is_a_spec.cr:159:5 in '->'
         from src/spec/example.cr:50:13 in 'internal_run'
         from src/spec/example.cr:38:16 in 'run'
         from src/spec/context.cr:20:23 in 'run'
         from src/spec/context.cr:20:23 in '->'
         from src/crystal/at_exit_handlers.cr:18:17 in 'main'
         from src/crystal/system/unix/main.cr:10:3 in 'main'
         from /usr/lib/libc.so.6 in '??'
         from /usr/lib/libc.so.6 in '__libc_start_main'
         from .build/compiler_spec in '_start'
         from ???

But taking a second look, I think it's just another case similar to the LLVM Context. Maybe a question for @ysbaddaden given he's more familiar with this part of the code.

Copy link
Collaborator

@ysbaddaden ysbaddaden Nov 24, 2025

Choose a reason for hiding this comment

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

The abstract EventLoop interface expects that the ::Socket type may not be defined, because we must always require "crystal/event_loop" but ::Socket is only defined when we explicitly require "socket".

Copy link
Member

@straight-shoota straight-shoota Dec 3, 2025

Choose a reason for hiding this comment

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

So the question is why is this def type checked even though it should never be callable when ::Socket is not defined?
The respective abstract def read(socket : ::Socket, slice : Bytes) : Int32 from event_loop/socket.cr should not be defined either when ::Socket is not defined.
It looks lile collateral damage from validating abstract def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32 from event_loop/file_descriptor.cr. It has the same name and is always defined. But it has a different signature from the socket overloads, so it should not be relevant here as there is a satisfying implementation for that signature.

Reduced example: abstract(param : Undefined) should not be an error because it's never called and abstract(param : Int32) implements the abstract def.

abstract class Abstract
  abstract def abstract(param : Int32)
end

class Concrete < Abstract
  def abstract(param : Int32)
  end

  # This method happens to have the same name as the abstract def, but with a
  # different signature that includes a type restiction which doesn't resolve.
  def abstract(param : Undefined)
  end
end

@HertzDevil
Copy link
Contributor

just def foo(x) to satisfy abstract def foo(x : U) : U forall U

IMO this should be legal with or without a return type in the abstract def, same goes for more specific pairs like def foo(x : Indexable) versus abstract def foo(x : Array(T)) forall T. As it stands right now, this PR now forbids those.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Nov 24, 2025

I reverted 9e97b5b, but kept the specs/added a few more. This bring it back with behavior that exists on master. Ultimately I think we'd want to prevent def transform(x : Int32) : Int32 from being considered valid for abstract def transform(x : Array(T)) forall T. But that can be a follow up at some point as it was already considered valid even before this PR.

@straight-shoota

This comment was marked as off-topic.

@HertzDevil

This comment was marked as off-topic.

return false
end

return true if restriction.global?
Copy link
Member

@straight-shoota straight-shoota Dec 3, 2025

Choose a reason for hiding this comment

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

So the question is why is this def type checked even though it should never be callable when ::Socket is not defined?
The respective abstract def read(socket : ::Socket, slice : Bytes) : Int32 from event_loop/socket.cr should not be defined either when ::Socket is not defined.
It looks lile collateral damage from validating abstract def read(file_descriptor : Crystal::System::FileDescriptor, slice : Bytes) : Int32 from event_loop/file_descriptor.cr. It has the same name and is always defined. But it has a different signature from the socket overloads, so it should not be relevant here as there is a satisfying implementation for that signature.

Reduced example: abstract(param : Undefined) should not be an error because it's never called and abstract(param : Int32) implements the abstract def.

abstract class Abstract
  abstract def abstract(param : Int32)
end

class Concrete < Abstract
  def abstract(param : Int32)
  end

  # This method happens to have the same name as the abstract def, but with a
  # different signature that includes a type restiction which doesn't resolve.
  def abstract(param : Undefined)
  end
end

Normalize type naming in specs
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:compiler:semantic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolving types for AbstractDefChecker seems broken

4 participants