Restrict argument types having a default value #3834

makenowjust · 2017-01-04T10:48:32Z

For example:

def foo(x : String = 1)
  x
end

foo

I think above code is invalid, but current compiler accepts it.
This pull request fixes it.

Another effect, below code works fine now:

def foo(x : T.class = String) forall T
  {{ T }}
end

foo # => String

(Before, we got "undefined constant T" error.)

Implementation point is introducing TypeRestrict fictitious node and applying it when instantiate a method with arguments having a default value.

For example: def foo(x : String = 1) x end foo I think above code is invalid, but current compiler accepts it. This commit fixes it. Another effect, below code works fine now: def foo(x : T.class = String) forall T {{ T }} end foo # => String (Before, we got "undefined constant T" error)

asterite · 2017-01-04T14:08:10Z

src/compiler/crystal/semantic/ast.cr

+  # Fictitious node to represent a type restriction
+  #
+  # It is used for type restrection of method arguments.
+  class TypeRestrict < ASTNode


Maybe call this TypeRestriction?

Ok. I'll fix.

asterite · 2017-01-04T14:08:26Z

src/compiler/crystal/semantic/ast.cr

+  #
+  # It is used for type restrection of method arguments.
+  class TypeRestrict < ASTNode
+    getter value


Maybe call this node?

But, if this is node, we use node.node in MainVisitor#visit.

TypeRestriction's semantics looks like Cast, so I think it is good that these getters rename same as Cast.

Sounds good 👍

asterite · 2017-01-04T14:15:43Z

src/compiler/crystal/semantic/main_visitor.cr

+
+      value.accept self
+
+      unless context = match_context


I'm thinking, maybe there's a different way to do this. The problem with this approach is that the type is checked once here (below). But what happens if the type of value changes later? (rare, but can happen)

Another way to do this is to do something like:

# This line is currently in `visit(node : Path)`, so we could probably extract it to a helper method restriction_type = (@path_lookup || @scope || @current_type).lookup_type_var(node, free_vars: @free_vars) unless restriction_type.is_a?(Type) node.raise "restriction must be a type, not #{restriction_type}" end # With this we make the node always have this type, and raise an error if it's not (also after a change) value.freeze_type = restriction_type

In this way I think there's no need to change the visitor to have a MatchContext instead of free_vars.

Could you try this change and see if it works?

freeze_type looks good. But, I think this way doesn't solve def foo(x : T.class = String) problem, and fixing this problem needs real call of restrict method, so match_context is needed.

The problem with this approach is that the type is checked once here (below). But what happens if the type of value changes later? (rare, but can happen)

Could you show me sample code?

If the default value is a method call. But don't worry, this case won't be possible in the future, and right now it's actually very to trigger that. I think your implementation is more than good, so let's leave it like that :-)

asterite · 2017-01-04T14:17:27Z

@makenowjust This is excellent, thank you!

I made a comment about a slightly different way of implementing this, but the basic idea you provided is really good. In fact this fixes #2636

asterite · 2017-01-04T14:18:53Z

src/compiler/crystal/semantic/ast.cr

+  #
+  # It is used for type restrection of method arguments.
+  class TypeRestriction < ASTNode
+    getter value


@makenowjust Ah, OK. Let's leave it as value for now :-)

Sija · 2017-01-04T14:24:31Z

src/compiler/crystal/semantic/to_s.cr

@@ -44,7 +44,7 @@ module Crystal
      false
    end

-    def visit(node : TypeRestrict)
+    def visit(node : TypeRestriction)
      @str << "# type restrict: "


leftover typo: restrict -> restriction

bcardiff · 2017-01-04T16:05:57Z

Regarding #3834 (comment), I would allow method calls in default methos so this things still work def foo(seed = T.default).

It make me suspicious that a node (even fictitious one) is added. Could the issue be resolved by enforcing that, when instantiating a def, the default value is_a? value of the type restriction (if any)?

asterite · 2017-01-04T18:04:45Z

src/compiler/crystal/semantic/main_visitor.cr

+      if type = obj.type.restrict(to, context)
+        node.type = type
+      else
+        node.raise "can't restrict #{obj.type} to #{to}"


A spec is missing for this (the specs you added seem to be testing other things)

Yes. I just added.

Sija · 2017-01-04T18:29:25Z

yup, 👍 for keeping method calls in default argument values.

class Foo
  protected def default_name
    preview? && "foo-preview" || "foo"
  end

  def save(name = default_name)
    # ...
  end
end

makenowjust · 2017-01-05T00:26:00Z

@bcardiff @Sija I think all method calls in default value aren't denied, and some pathological method calls are denied (I considered such an example, but I can't imagine it.)

bcardiff · 2017-01-05T00:28:23Z

@makenowjust Sorry, could you rephrase or expand? I don't follow you.

makenowjust · 2017-01-05T00:33:04Z

@bcardiff I don't think all method calls in default value are banned by compiler, so don't worry.

Sija · 2017-01-05T01:16:01Z

spec/compiler/semantic/def_spec.cr

@@ -171,6 +171,16 @@ describe "Semantic: def" do
      "undefined method"
  end

+  it "errors when type restriction is incompatible with default value" do


I'd rephrase it: errors when default value is incompatible with type restriction

asterite · 2017-01-06T12:32:29Z

@makenowjust Thank you for this!

makenowjust force-pushed the fix/crystal/type-restrict-default branch from 27de8e6 to 247407f Compare January 4, 2017 10:51

asterite reviewed Jan 4, 2017

View reviewed changes

Rename TypeRestrict to TypeRestriction

5de29d2

asterite reviewed Jan 4, 2017

View reviewed changes

Sija reviewed Jan 4, 2017

View reviewed changes

makenowjust added 2 commits January 4, 2017 23:38

Rename TypeRestrict properties same as Cast

69709e7

Fix typo

9422c51

makenowjust changed the title ~~Restrict arguments having a default value~~ Restrict argument types having a default value Jan 4, 2017

asterite reviewed Jan 4, 2017

View reviewed changes

Add spec for failed type restriction

34fd743

Sija reviewed Jan 5, 2017

View reviewed changes

Rephrase

1398ad0

asterite merged commit 982bbc5 into crystal-lang:master Jan 6, 2017

asterite added topic:compiler kind:feature labels Jan 6, 2017

asterite added this to the 0.20.4 milestone Jan 6, 2017

asterite mentioned this pull request Jan 6, 2017

Typecheck default values if unusued #2636

Closed

makenowjust deleted the fix/crystal/type-restrict-default branch January 6, 2017 12:50

maiha mentioned this pull request Jan 6, 2017

Default argument types now must match their restriction stefanwille/crystal-redis#33

Merged

samfrench mentioned this pull request Jan 20, 2017

Update integer variable type comandeo/crystal-memcached#19

Merged

Sija mentioned this pull request Sep 13, 2017

Type inference and generics when dealing with default nil #2456

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict argument types having a default value #3834

Restrict argument types having a default value #3834

makenowjust commented Jan 4, 2017 •

edited

Loading

asterite Jan 4, 2017

makenowjust Jan 4, 2017

asterite Jan 4, 2017

makenowjust Jan 4, 2017

makenowjust Jan 4, 2017

asterite Jan 4, 2017

asterite Jan 4, 2017

makenowjust Jan 4, 2017

asterite Jan 4, 2017

asterite commented Jan 4, 2017

asterite Jan 4, 2017

Sija Jan 4, 2017 •

edited

Loading

makenowjust Jan 4, 2017

bcardiff commented Jan 4, 2017

asterite Jan 4, 2017

makenowjust Jan 5, 2017

Sija commented Jan 4, 2017

makenowjust commented Jan 5, 2017 •

edited

Loading

bcardiff commented Jan 5, 2017

makenowjust commented Jan 5, 2017

Sija Jan 5, 2017

makenowjust Jan 5, 2017

asterite commented Jan 6, 2017

Restrict argument types having a default value #3834

Restrict argument types having a default value #3834

Conversation

makenowjust commented Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Jan 4, 2017

Choose a reason for hiding this comment

Sija Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija commented Jan 4, 2017

makenowjust commented Jan 5, 2017 • edited Loading

bcardiff commented Jan 5, 2017

makenowjust commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Jan 6, 2017

makenowjust commented Jan 4, 2017 •

edited

Loading

Sija Jan 4, 2017 •

edited

Loading

makenowjust commented Jan 5, 2017 •

edited

Loading