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

Semantic: correctly guess ivar type from splat argument #6648

Merged
merged 2 commits into from Sep 3, 2018

Conversation

Projects
None yet
4 participants
@MakeNowJust
Contributor

MakeNowJust commented Sep 2, 2018

Caught from https://twitter.com/n_siena/status/1026021219714269184 (Japanese).

Before this PR the type of ivar @foo of def initialize(*@foo : Int32) is guessed to Int32. It causes very strange error. For example:

class Foo
  def initialize(*@foo : Int32)
  end
end

Foo.new(1)
# Error: instance variable '@foo' of Foo must be Int32, not Tuple(Int32)

I think it should not be guessed to any types because this restriction means variable-length tuple and we have no type for it. Indexable(Int32) is one of generic types of variable-length tuple, but it is not perfect. So, it should be specified explicitly if needed.

Nevertheless, @foo type of def initialize(*@foo : *{Int32}) can be guessed to {Int32} easily. It should be guessed.

A double-splat and ivar has the same problems. This PR fixes them. Now we get this error and result.

class Foo
  def initialize(*@foo : Int32)
  end
end

Foo.new(1)
# Error: Can't infer the type of instance variable '@foo' of Foo
alias T1 = {Int32}

class Foo
  getter foo
  def initialize(*@foo : *T1)
  end
end

pp Foo.new(1).foo.class # => {Int32}
Semantic: correctly guess ivar type from splat argument
Before this commit the type of ivar `@foo` of `def initialize(*@foo : Int32)`
is guessed to `Int32`. It is very strange.

It should not be guessed to any types because this restriction means
variable-length tuple and we have no type for it. `Indexable(Int32)` is one
of generic types of variable-length tuple, but it is not perfect. So, it
should be specified explicitly if needed.

However, `@foo` type of `def initialize(*@foo : *{Int32})` can be guessed
to `{Int32}` easily. It should be guessed and it is useful for
`def initialize(*@foo : *T)` case.

A double-splat and ivar has the same problems. This commit fixes them.

@MakeNowJust MakeNowJust force-pushed the MakeNowJust:fix/crystal/correctly-guess-ivar-type-from-splat-argument branch from 3ca3ca4 to 33b816f Sep 2, 2018

@@ -800,6 +803,20 @@ module Crystal
if arg = args_hash[node.name]?
# If the argument has a restriction, guess the type from it
if restriction = arg.restriction
# It is for something like `def foo(*@foo : *T)`.
if @splat == arg

This comment has been minimized.

@asterite

asterite Sep 3, 2018

Contributor

I'd do @splat.same?(arg)

This comment has been minimized.

@MakeNowJust

MakeNowJust Sep 3, 2018

Contributor

Thank you. I think so too. I'll fix it later.

Show resolved Hide resolved src/compiler/crystal/semantic/type_guess_visitor.cr

@RX14 RX14 requested a review from asterite Sep 3, 2018

@RX14

RX14 approved these changes Sep 3, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Sep 3, 2018

(sorry for the review request, I left the issue tab open overnight and github didn't update)

@RX14

RX14 approved these changes Sep 3, 2018

@RX14 RX14 merged commit 8888c79 into crystal-lang:master Sep 3, 2018

4 checks passed

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

@RX14 RX14 added this to the 0.27.0 milestone Sep 3, 2018

@MakeNowJust MakeNowJust deleted the MakeNowJust:fix/crystal/correctly-guess-ivar-type-from-splat-argument branch Sep 3, 2018

ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018

Semantic: correctly guess ivar type from splat argument (crystal-lang…
…#6648)

* Semantic: correctly guess ivar type from splat argument

Before this commit the type of ivar `@foo` of `def initialize(*@foo : Int32)`
is guessed to `Int32`. It is very strange.

It should not be guessed to any types because this restriction means
variable-length tuple and we have no type for it. `Indexable(Int32)` is one
of generic types of variable-length tuple, but it is not perfect. So, it
should be specified explicitly if needed.

However, `@foo` type of `def initialize(*@foo : *{Int32})` can be guessed
to `{Int32}` easily. It should be guessed and it is useful for
`def initialize(*@foo : *T)` case.

A double-splat and ivar has the same problems. This commit fixes them.

* Use `same?` instead `==`

crystal-lang#6648 (comment)
@bcardiff

This comment has been minimized.

Member

bcardiff commented Oct 30, 2018

I'm finding some issues with this change.

Before this PR methods with var args methods where able to be restricted without problems:

def concat(*args : String)
  args.join(", ")
end

This means args[i] is of type String. Same happen with **args : String.

I understand that the guess visitor was not handling def initialize(*@args : String) since there is not straight solution to type @args that will satisfy all possible invocations of that method.

I don't understand how this PR improves that situation.

From the added specs the following works

class Foo
  def initialize(*@foo : *{Int32})
  end
end

Foo.new(1)

But Foo.new(1, 2) no longer compiles.
Making def initialize(*foo : {Int32, Int32}) will make Foo.new(1, 2) pass, but not Foo.new(1).
So with the proposed change the arg is no longer of variable length.

In the case of named argument I don't understand why would we need to mention a name in the restriction if the only thing we should express, at most, is the type of all the values. (Look at def initialize(**@foo : **{foo: Int32}) in the specs)

Since the problem is how instance vars can (or can't) be initialized for splatted argument I would propose:

  • revert this PR
  • disallow using ivar initializers in splatted arguments
  • keep the m(*args : String) and m(**args : String) syntax as the only constructs that could appear for splatted arguments to express the types of all the values.

Unless of course, someone helps me see what I am misunderstanding here.

@MakeNowJust

This comment has been minimized.

Contributor

MakeNowJust commented Oct 31, 2018

@bcardiff What is problem? I didn't disable m(*args : String) and m(**args : String) syntaxes in this PR. They are still available.

@asterite

This comment has been minimized.

Contributor

asterite commented Oct 31, 2018

@bcardiff the *{Int32} syntax means it's a splat of a tuple of one element. That's why the type can be guessed.

(I don't know how useful is that, but that's how it works)

@RX14

This comment has been minimized.

Member

RX14 commented Oct 31, 2018

@bcardiff

class Foo
  def initialize(*@foo : *{Int32, String})
  end
end

means

class Foo
  @foo : {Int32, String}
  def initialize(*foo)
    @foo = foo
  end
end

This seems useless at first but is useful when you have class Foo(*T) to let the compiler guess the generic type correctly from the constructor:

class Foo(*T)
  def initialize(*@args : *T)
    p typeof(@args)
  end
end

Foo.new("name", 1, 6.0, "foo")

This PR doesn't change the semantics of the above code snippet at all. It worked before and it worked after. It just takes care to allow that case specifically while disallowing non-splat type restrictions. At least thats what I think from reading the code.

@RX14

This comment has been minimized.

Member

RX14 commented Oct 31, 2018

BTW: another bug https://carc.in/#/r/5e8b :)

@bcardiff

This comment has been minimized.

Member

bcardiff commented Oct 31, 2018

Ok, I was missing that the use case was splatting over a type argument.

I still don't like it that much. I can't see the value of using splats in this scenario, and if splats are removed then we can pass and store directly the tuple.

Most likely is because I only think of restricting splats by the type of values (not length, not names).

Thanks for clarifying everybody.

@RX14

This comment has been minimized.

Member

RX14 commented Nov 2, 2018

@bcardiff how would you achieve this interface without using splats??

@bcardiff

This comment has been minimized.

Member

bcardiff commented Nov 2, 2018

I think it can't.

The itch comes from the fact that the number of arguments changes in a very strict way inside a generic family.

Instantiated generics clases of course have incompatible interfaces if the T appears in the type restriction. But that usually does not predicate over the number or names of argument.

But it's an itch, I am not sure if breaks something formally.

@asterite

This comment has been minimized.

Contributor

asterite commented Nov 2, 2018

In general generic types over a splat are a big itch of me too. It's nice that it works but it's curious that no other language provides this and you have Tuple1, Tuple2, etc. in other languages. And I think the reason is this itch that there's something wrong (magical) about it.

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Semantic: correctly guess ivar type from splat argument (crystal-lang…
…#6648)

* Semantic: correctly guess ivar type from splat argument

Before this commit the type of ivar `@foo` of `def initialize(*@foo : Int32)`
is guessed to `Int32`. It is very strange.

It should not be guessed to any types because this restriction means
variable-length tuple and we have no type for it. `Indexable(Int32)` is one
of generic types of variable-length tuple, but it is not perfect. So, it
should be specified explicitly if needed.

However, `@foo` type of `def initialize(*@foo : *{Int32})` can be guessed
to `{Int32}` easily. It should be guessed and it is useful for
`def initialize(*@foo : *T)` case.

A double-splat and ivar has the same problems. This commit fixes them.

* Use `same?` instead `==`

crystal-lang#6648 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment