[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385

eregon · 2016-12-14T16:39:08Z

@jruby/truffle Please share your opinion.

I think the code is in general better with these changes, undefined can't get away and can only be used as a marker for "not provided argument" which is the main purpose.

The only case which might be worth supporting would be to allow it is case statements, which could be done more easily with something like:

case arg
when String
  ...
when Undefined
  ...
end

and then Undefined would have a special === that only matches the right instance.
I don't think it's worth it, usually logic is just duplicated for the undefined case, so it might as well be normalized before, with arg = sensible_default if undefined.equal?(arg)

* The condition is impossible.

…y_convert_to_encoding.

…erter#initialize.

…ided.INSTANCE in Java. * "if undefined" and "undefined.call" are unsupported, on purpose. undefined arguments should be checked early and not carry the undefined around.

chrisseaton · 2016-12-14T18:24:44Z

truffle/src/main/ruby/core/array.rb

      left = Rubinius::Type.coerce_to_collection_length one
      left += size if left < 0
      left = 0 if left < 0

-      if two and !undefined.equal?(two)
+      if !undefined.equal?(two) and two


Why and rather than &&?

I'm just used to and over && as it looks less cryptic and most of the time there is no need for the higher precedence of &&. But either are fine. The original code used and here.

afaik: Original intention is that && is a logical operator and and is a control flow operator. They are not meant to be interchangeable. Following is interchangeable, although the first is much more common.

return :nothing if test.nil? test.nil? and return :nothing

Just my opinion: second one looks like Perl or PHP and and is needed for assignment in conditionals.

Convention is to add parenthesis if (a = true) && b.nil? to make the assignment stand out more. I like test or raise something form since it puts the test first not the unimportant exception as in raise something unless test. Anyway this is not important, it's opinionated and gain is just minor readability improvements.

nirvdrum · 2016-12-14T22:38:21Z

truffle/src/main/java/org/jruby/truffle/core/CoreLibrary.java

@@ -278,7 +279,7 @@
    private final GlobalVariables globalVariables;
    private final DynamicObject mainObject;
    private final DynamicObject nilObject;
-    private final DynamicObject rubiniusUndefined;
+    private final Object rubiniusUndefined;


Probably out of scope, but I think we can rename this now.

Yes, some cleanup can be done after, since we can get rid of this whole field and guard.

nirvdrum · 2016-12-14T22:41:19Z

truffle/src/main/ruby/core/complex.rb

@@ -53,9 +53,10 @@ class Complex < Numeric
  undef_method :i

  def self.convert(real, imag = undefined)
-    if real.equal?(nil) || imag.equal?(nil)
+    if nil.equal?(real) || nil.equal?(imag)


Since you modified the order here, is there any reason to not just use real.nil? and imag.nil? ?

they might be undefined, and calling methods on undefined does not work, on purpose.

Even though calling literal.equal?(some_var) is less nice, it's actually better for the semantics, otherwise you could invoke a method on a user object (more important for == than equal? which is rarely redefined). We could play some tricks in core, but I'd rather avoid too much magic unless needed.

Okay. This seems like a rule someone (probably me) is bound to forget.

Yeah, there are a few places in core where it's not like this.
For undefined you can't forget as it will trigger an assert if you write it in the other way :)

nirvdrum · 2016-12-14T22:44:26Z

truffle/src/main/ruby/core/encoding.rb

@@ -141,7 +141,7 @@ class Converter
    def self.asciicompat_encoding(string_or_encoding)
      encoding = Rubinius::Type.try_convert_to_encoding string_or_encoding

-      return if not encoding or undefined.equal? encoding
+      return unless encoding


Doesn't BooleanCastNode need to be updated to handle the undefined object in order for this to work?

The return value of try_convert_to_encoding was changed to avoid this precisely :)
false is returned where undefined was before in that method.

undefined used to be "truthy" so I did not dare to accept it in BooleanCastNode.

Wow. I would have guessed it was "falsey."

nirvdrum · 2016-12-14T22:47:41Z

truffle/src/main/ruby/core/io.rb

@@ -588,13 +588,12 @@ def self.foreach(name, separator=undefined, limit=undefined, options=undefined)

    name = Rubinius::Type.coerce_to_path name

+    separator = $/ if undefined.equal?(separator)


It probably doesn't matter all that much, but this seems to do extra processing that we didn't have before and doesn't seem to make things all that much clearer.

Nevermind. I see this is the case issue you mentioned in the PR description.

eregon added the truffle label Dec 14, 2016

eregon added this to the truffle-dev milestone Dec 14, 2016

eregon requested review from nirvdrum, pitr-ch, chrisseaton and bjfish December 14, 2016 16:39

eregon added 13 commits December 14, 2016 18:04

[Truffle] Remove useless check.

cd2d5a9

* The condition is impossible.

[Truffle] Rearrange undefined checks so they are done first.

a1dbf12

[Truffle] Avoid using undefined in a couple Thread methods.

523d42d

[Truffle] Make explicit a boolean check against undefined.

911cbb9

[Truffle] Use false instead of undefined when conversion failed in tr…

1653b93

…y_convert_to_encoding.

[Truffle] Proper order for check against undefined.

f832a33

[Truffle] Use the natural default value for options in Encoding::Conv…

21b59b5

…erter#initialize.

[Truffle] Avoid propagating undefined too far in Complex.convert.

3b62127

[Truffle] Avoid undefined in case in String#encode!.

8d37436

[Truffle] Avoid undefined in case in io.rb.

a2d31cb

[Truffle] Don't try to convert undefined to a Hash.

69c3c61

[Truffle] Use a default of nil for mode passed to IO.normalize_options.

9f08968

[Truffle] Make undefined in Ruby code the exact same thing as NotProv…

3bd4249

…ided.INSTANCE in Java. * "if undefined" and "undefined.call" are unsupported, on purpose. undefined arguments should be checked early and not carry the undefined around.

eregon force-pushed the truffle-undefined-not-provided branch from a714351 to 3bd4249 Compare December 14, 2016 17:04

chrisseaton approved these changes Dec 14, 2016

View reviewed changes

bjfish approved these changes Dec 14, 2016

View reviewed changes

eregon force-pushed the truffle-undefined-not-provided branch from 7ac33a3 to 3bd4249 Compare December 14, 2016 18:43

nirvdrum reviewed Dec 14, 2016

View reviewed changes

nirvdrum approved these changes Dec 15, 2016

View reviewed changes

pitr-ch approved these changes Dec 15, 2016

View reviewed changes

eregon merged commit da53955 into jruby:truffle-head Dec 15, 2016

eregon deleted the truffle-undefined-not-provided branch December 15, 2016 15:27

enebo added this to the Non-Release milestone Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385

[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385

eregon commented Dec 14, 2016 •

edited

chrisseaton Dec 14, 2016

eregon Dec 14, 2016

pitr-ch Dec 15, 2016

eregon Dec 15, 2016 •

edited

pitr-ch Dec 15, 2016

nirvdrum Dec 14, 2016

eregon Dec 15, 2016

nirvdrum Dec 14, 2016

eregon Dec 15, 2016

eregon Dec 15, 2016

nirvdrum Dec 15, 2016

eregon Dec 15, 2016

nirvdrum Dec 14, 2016

eregon Dec 15, 2016 •

edited

eregon Dec 15, 2016

nirvdrum Dec 15, 2016

nirvdrum Dec 14, 2016

nirvdrum Dec 14, 2016

		@@ -588,13 +588,12 @@ def self.foreach(name, separator=undefined, limit=undefined, options=undefined)

		name = Rubinius::Type.coerce_to_path name

		separator = $/ if undefined.equal?(separator)

[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385

[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385

Conversation

eregon commented Dec 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon Dec 15, 2016 • edited

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

Choose a reason for hiding this comment

eregon Dec 15, 2016 • edited

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

eregon commented Dec 14, 2016 •

edited

eregon Dec 15, 2016 •

edited

eregon Dec 15, 2016 •

edited