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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix restriction of valid type vs free vars #7536

Conversation

Projects
None yet
3 participants
@bew
Copy link
Contributor

commented Mar 10, 2019

Fixes #6446


def bar(a : T.class) forall T
  puts "Hello from T"
end
  
def bar(a : Int32.class)
  puts "Hello from Int32"
end

# Before:
bar(Int32) # "Hello from T"

# After
bar(Int32) # "Hello from Int32"

Compiler hacking is fun 馃帀

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

I'll review it tomorrow.

@asterite
Copy link
Member

left a comment

@bew Another great PR! Thank you!

@asterite asterite added this to the 0.28.0 milestone Mar 12, 2019

@bew

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Thanks for the review 馃槂

Just a note, since I currently can't check from the restriction method if the type is really a free var, this happens:

https://carc.in/#/r/6hcr

# No free vars here, and Foo doesn't exist
def bar(a : Foo.class)
  puts "Hello from Foo"
end
  
def bar(a : Int32.class)
  puts "Hello from Int32"
end

# Before:
bar(Int32) # Error: undefined constant Foo

# After
bar(Int32) # "Hello from Int32"
@asterite

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@bew Yes, I'm not sure. I think it's bad that if you don't invoke the method you don't get the "undefined constant Foo" either way. We should probably have a pass to compute those types before analyzing calls, and then in the restriction you would know whether a type is a free var or not. For now maybe this is fine.

@bew

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

I think the new behavior is fine.

I'm not sure how you could fix that, since you could have:

def bar(a : Foo.class)
  puts "Hello from Foo"
end
  
def bar(a : Int32.class) # restriction happens here when this method is processed
  puts "Hello from Int32"
end

record Foo # define Foo here!

bar(Foo) # => "Hello from Foo"

But maybe if there's first a pass to find all types, then a pass to process al methods that could do it, but there will be other problems I think, like macros that creates types / create methods / use @type.methods etc..

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@bew Macros run on the very first pass. This second pass would check the types of all method arguments to see if they exist (they must either be in scope or declared as free variables). I'm almost sure it's something doable. But it's tricky because methods are also added on the first pass (though they are not type-checked there, and this is why it's tricky to compute which one is stricter than the other). We should probably also have some context of is_restriction? to know whether something is a free var.

But let's merge this for now.

@asterite asterite merged commit 11b2625 into crystal-lang:master Mar 13, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bew bew deleted the bew:bugfix/fix-restriction-of-valid-type-vs-free-vars branch Mar 13, 2019

@bew

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

We should probably also have some context of is_restriction? to know whether something is a free var.

Yes that's what I was thinking, for example the owner param could be a ctx param instead, with an owner field, but also a def field representing the current def is any.

@bew

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Actually I've found a much simpler solution while playing with #7543:

class Metaclass
  def restriction_of?(other : Metaclass, ctx)
    self.name.restriction_of?(other.name, ctx)
  end
end

Done in #7580

urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 20, 2019

Urde Graven
Merge branch 'master' of github.com:crystal-lang/crystal
* 'master' of github.com:crystal-lang/crystal:
  Change the font-weight used in Playground (crystal-lang#7552)
  Fix formatting absolute paths (crystal-lang#7560)
  Refactor IO::Syscall as IO::Evented (crystal-lang#7505)
  YAML: fix test checking serialization of slices for libyaml 0.2.2 (crystal-lang#7555)
  Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial comparability (crystal-lang#6611)
  Update `to_s` and `inspect` to have similar signatures accross the stdlib (crystal-lang#7528)
  Fix restriction of valid type vs free vars (crystal-lang#7536)
  Rewrite macro spec without executing a shell command (crystal-lang#6962)
  Suggest `next` when trying to break from captured block  (crystal-lang#7406)
  Fix GenericClassType vs GenericClassInstanceType restrictions (crystal-lang#7537)
  Add human readable formatting for numbers (crystal-lang#6314)
  Add command and args to execvp error message (crystal-lang#7511)
  Implement Set#add? method (crystal-lang#7495)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.