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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling an NamedTuple#to_h on an empty now has meaningful error message #4076

Merged
merged 2 commits into from
Mar 10, 2017
Merged

Calling an NamedTuple#to_h on an empty now has meaningful error message #4076

merged 2 commits into from
Mar 10, 2017

Conversation

karlseguin
Copy link
Contributor

@karlseguin karlseguin commented Feb 24, 2017

switched from making it return {} => Nil to just improving the compile-time error message

@bcardiff
Copy link
Member

@karlseguin do you have any scenario for this?

I am not convinced the Symbol => Nil will be comfortable when you potentially will expect a hash of other type if the tuple is not empty. NamedTuple#to_a for example return Array(Tuple(NoReturn, NoReturn)).

Tuple does not have a #to_a but if it had, the same problem will be present there. You will sometimes have a Array(Nil) or and Array(T1 | T2 | ...). So, you will still be needing some case over time.

Another alternative could be to do something as follows. Depending on your use case it might be useful or not.

def some_key?(**opt)
  if opt.empty?
    :empty
  else
    opt.each do
      return opt.keys[0]
    end
  end
end

some_key? foo: 1 # => :foo
some_key?        # => :empty

@karlseguin
Copy link
Contributor Author

karlseguin commented Feb 24, 2017

I'm sure I could come up with a different (better) way to solve my specific problem. But it seems weird to me that calling a method on an instance would give a compiler error based on the state of that instance. On the other hand, I guess the fact the they're known at compile time means that we can do the check at compile time.

I definitely think the current behavior is wrong (if for no other reason as being an unhelpful error message), so, we could:

1 - Change the default type to something else, but what?
2 - Raise a exception
3 - Improve the compile-time error message

My case was factories for creating tests objects that looked like:

def user(args = NamedTuple.new())
   # somewhere args.to_h was being called

the simplest solution in this case would be to make it nullable.

@karlseguin
Copy link
Contributor Author

For reference, this is how it's currently being handled

in /opt/crystal/src/named_tuple.cr:402: macro didn't expand to a valid program, it expanded to:

================================================================================
--------------------------------------------------------------------------------
   1. 
   2.       {
   3.         
   4.       }
   5.     
--------------------------------------------------------------------------------
Syntax error in expanded macro: macro_4610676704:2: for empty hashes use '{} of KeyType => ValueType'

      {
      ^

================================================================================

    {% begin %}

@RX14
Copy link
Contributor

RX14 commented Feb 24, 2017

Why not make it return nil. Because this is done via macros, I don't think that you will actually have to deal with to_h returning nil until there's an empty tuple in the union you're calling to_h on.

@karlseguin
Copy link
Contributor Author

I changed it to give a more meaningful compile-time error. This seems like the least risky improvement. I couldn't figure out how to get it under test though.

@karlseguin karlseguin changed the title Calling an NamedTuple#to_h on an empty tuple returns {} of Symbol => Nil Calling an NamedTuple#to_h on an empty now has meaningful error message Feb 25, 2017
@Sija
Copy link
Contributor

Sija commented Feb 26, 2017

👍 for returning nil.

Otherwise scenario below becomes impossible:

def foo(**options)
  options.to_h
end

@asterite
Copy link
Member

to_h returns hash, not nil. Original PR was OK

@bcardiff
Copy link
Member

I still think that raising in compile time is the right thing to do.

Dealing with a hash version of a named tuple is far from a type safe thing.
If the developer want to to something with the hash coming from options.to_h:

def foo(**options)
  # use options.to_h
end

there should be some basic assumptions of the types of the values. for example: all integers

def foo(**options)
  options.to_h.values.sum
end

So foo(a: 1, b: 2) # => 3 works. And foo(a: 1, b: "2") fails to compile.
foo also fails to, before this PR with a more cryptic error message.
So failing with a clear "Can't convert an empty NamedTuple to a Hash" is an improvement.

What happens if we want to return a hash (of some type) for the empty tuple?
For hash type we choose Symbol => Nil, NoReturn => NoReturn, etc there will be a case that fails to compile when the original named tuple is empty due to a type mismatch between the hash for the empty tuple ad the hash for the non empty tuple.

So, in the sample case of returning the sum of the values of the named tuple the programer is left with two options.

a) overload:

def foo
  0
end

def foo(**options)
  options.to_h.values.sum
end

b) split the case with and weird is_a? (I think there is no other way to express the empty tuple type).

def foo(**options)
  if options.is_a?(typeof(NamedTuple.new))
    0
  else
    options.to_h.values.sum
  end
end

@bcardiff
Copy link
Member

@karlseguin I authored the changeset, there was an unneeded begin/end block since now there is a macro if. After CI pass this should be ready to merge.

I also added a spec for this (yet upon #2391 it could be moved to the std)

@bcardiff bcardiff added this to the Next milestone Mar 10, 2017
@bcardiff bcardiff merged commit 3e1abe3 into crystal-lang:master Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants