-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Better handle free vars and unknown types in abstract defs #16407
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
base: master
Are you sure you want to change the base?
Changes from all commits
583192c
9e97b5b
22f8678
9440bab
5ae3e35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,7 +116,7 @@ class Crystal::AbstractDefChecker | |
|
|
||
| if implements?(target_type, ancestor_type, a_def, def_free_vars, base, method, method_free_vars) | ||
| unless implemented | ||
| check_return_type(target_type, ancestor_type, a_def, base, method) | ||
| check_return_type(target_type, ancestor_type, a_def, def_free_vars, base, method, method_free_vars) | ||
| implemented = true | ||
| end | ||
|
|
||
|
|
@@ -264,19 +264,66 @@ class Crystal::AbstractDefChecker | |
| return false if r1 && !r2 | ||
| if r2 && r1 && r1 != r2 | ||
| # Check if a1.restriction is contravariant with a2.restriction | ||
| rt1 = nil | ||
| rt2 = nil | ||
|
|
||
| begin | ||
| rt1 = t1.lookup_type(r1, free_vars: free_vars1) | ||
| rescue Crystal::TypeException | ||
| end | ||
|
|
||
| begin | ||
| rt2 = t2.lookup_type(r2, free_vars: free_vars2) | ||
| return false unless rt2.implements?(rt1) | ||
| rescue Crystal::TypeException | ||
| # Ignore if we can't find a type (assume the method is implemented) | ||
| return true | ||
| end | ||
|
|
||
| if rt1 && rt2 | ||
| # Both types resolved - check compatibility | ||
| return false unless rt2.implements?(rt1) | ||
| elsif !rt2 | ||
| # Only implementation side couldn't be resolved | ||
| if !can_lookup?(r2, free_vars2, t2) | ||
| report_undefined_type(r2) | ||
| end | ||
| elsif !rt1 | ||
| # Only abstract side couldn't be resolved | ||
| if !can_lookup?(r1, free_vars1, t1) | ||
| report_undefined_type(r1) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| true | ||
| end | ||
|
|
||
| # Check if a restriction can potentially be looked up (is a free var or might be resolved later) | ||
| private def can_lookup?(restriction : ASTNode, free_vars, type : Type? = nil) : Bool | ||
| return true unless restriction.is_a?(Path) | ||
|
|
||
| # For single-name paths, check if it's a free variable | ||
| if name = restriction.single_name? | ||
| # Check if it's in free_vars (forall parameters) | ||
| return true if free_vars && free_vars.has_key?(name) | ||
|
|
||
| # Check if it's a type parameter of the enclosing generic type | ||
| if type && type.is_a?(GenericType) | ||
| return true if type.type_vars.includes?(name) | ||
| end | ||
|
|
||
| return false | ||
| end | ||
|
|
||
| return true if restriction.global? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The abstract EventLoop interface expects that the |
||
|
|
||
| # Other multi-segment paths can't be free variables | ||
| # Return false to indicate they should be reported as undefined if lookup failed | ||
| false | ||
| end | ||
|
|
||
| private def report_undefined_type(restriction : ASTNode) | ||
| report_error(restriction, "undefined constant #{restriction}") | ||
| end | ||
|
|
||
| def same_parameters?(m1 : Def, m2 : Def) | ||
| return false unless m1.args.size == m2.args.size | ||
|
|
||
|
|
@@ -318,12 +365,13 @@ class Crystal::AbstractDefChecker | |
|
|
||
| # Checks that the return type of `type#method` matches that of `base_type#base_method` | ||
| # when computing that information for `target_type` (`type` is an ancestor of `target_type`). | ||
| def check_return_type(target_type : Type, type : Type, method : Def, base_type : Type, base_method : Def) | ||
| def check_return_type(target_type : Type, type : Type, method : Def, method_free_vars, base_type : Type, base_method : Def, base_method_free_vars) | ||
| base_return_type_node = base_method.return_type | ||
| return unless base_return_type_node | ||
|
|
||
| original_base_return_type = base_type.lookup_type?(base_return_type_node) | ||
| original_base_return_type = base_type.lookup_type?(base_return_type_node, free_vars: base_method_free_vars) | ||
| unless original_base_return_type | ||
| return if can_lookup?(base_return_type_node, base_method_free_vars, base_type) | ||
| report_error(base_return_type_node, "can't resolve return type #{base_return_type_node}") | ||
| return | ||
| end | ||
|
|
@@ -340,8 +388,9 @@ class Crystal::AbstractDefChecker | |
| base_return_type_node.accept(replacer) | ||
| end | ||
|
|
||
| base_return_type = base_type.lookup_type?(base_return_type_node) | ||
| base_return_type = base_type.lookup_type?(base_return_type_node, free_vars: base_method_free_vars) | ||
| unless base_return_type | ||
| return if can_lookup?(base_return_type_node, base_method_free_vars, base_type) | ||
| report_error(base_return_type_node, "can't resolve return type #{base_return_type_node}") | ||
| return | ||
| end | ||
|
|
@@ -352,8 +401,9 @@ class Crystal::AbstractDefChecker | |
| return | ||
| end | ||
|
|
||
| return_type = type.lookup_type?(return_type_node) | ||
| return_type = type.lookup_type?(return_type_node, free_vars: method_free_vars) | ||
| unless return_type | ||
| return if can_lookup?(return_type_node, method_free_vars, type) | ||
| report_error(return_type_node, "can't resolve return type #{return_type_node}") | ||
| return | ||
| end | ||
|
|
||
There was a problem hiding this comment.
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
Contextis.