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

identical(double.NAN, double.NAN) should be ? #563

Closed
DartBot opened this issue Nov 22, 2011 · 19 comments
Closed

identical(double.NAN, double.NAN) should be ? #563

DartBot opened this issue Nov 22, 2011 · 19 comments
Assignees
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).

Comments

@DartBot
Copy link

DartBot commented Nov 22, 2011

This issue was originally filed by olov.las...@gmail.com


~/projects/dart % cat nan_eq.dart
main() {
  var nan = double.NAN;
  print("$nan == $nan: ${nan == nan}");
  print("$nan === $nan: ${nan === nan}");
}

~/projects/dart % dart nan_eq.dart
NaN == NaN: false
NaN === NaN: true

~/projects/dart % dart/frog/frogsh nan_eq.dart
NaN == NaN: false
NaN === NaN: false

Equality works as expected (like C and JS) and is consistent between the vm and frog/dartc.

Identity is inconsistent between the vm and frog/dartc.

@floitschG
Copy link
Contributor

There is probably a similar case for -0.0 and 0.0.
That is, on the VM I expect -0.0 !== 0.0, and on frog it is probably -0.0 === 0.0.

@DartBot
Copy link
Author

DartBot commented Nov 22, 2011

This comment was originally written by olov.lassu...@gmail.com


Correct, and in addition to that and perhaps a bit more surprising the VM considers -0.0 !== -0.0 (frog doesn't). Seems like another bug, though.

@DartBot
Copy link
Author

DartBot commented Nov 24, 2011

This comment was originally written by drfibonacci@google.com


Added Area-Compiler, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Mar 28, 2012

This comment was originally written by zundel@google.com


Probably will be an issue for dart2js so I moved it there.


Removed Area-Compiler label.
Added Area-Dart2JS label.

@kasperl
Copy link

kasperl commented Jun 12, 2012

Changed the title to: "NaN === NaN identity test differs between the vm and dart2js".

@DartBot
Copy link
Author

DartBot commented Jun 21, 2012

This comment was originally written by rice@google.com


This code fails on dart2js:

Expect.identical(double.NAN, double.NAN);

@rakudrama
Copy link
Member

Having the language be sensitive to the boxed state of floating point numbers is asking for trouble.

It is not possible to make double.NAN === double.NAN when compiled to JavaScript (without explicitly boxing almost every number) because JavaScript hides the identity of numbers and strings.

On the VM if we demand that double.NAN === double.NAN, then we cannot do many numeric optimizations because the 'box' of the object is observable and should the preserved by optimizations.

   var x = 1.0;
   var y1 = x + 2.3;
   var y2 = x + 2.3;
   ...
   print(y1 === y2);

If double is defined by the library and not by the language, the VM is obliged to box y1 and y2 separately since the optimizer should not change the observable behaviour of the program.

The same set of issues apply to small integers (different implementations may canonicalize small integers with different numbers of bits) and, thanks to JavaScript, strings.

JavaScript made a good call here for the programmer - it is not possible to tell if a number or string is boxed or not and programs are more consistent, more reliable and have more opportunities for optimization because of it. The cost is that === is nearly as expensive as == in common cases.

I believe we are going to try to sidestep this issue by encouraging the use of == by removing === and providing a function identical(x,y) in its place. I have not seen the full rationale which I would expect explains the compromises. I think the spec needs some language to do one of the following:
  1. explain when the optimizer can change the result of identical(x, y).
  2. or say that identical(x, y) is more than bitwise pointer equality to ensure identical() and other identity sensitive operations cannot tell the difference between different objects of certain types.


cc @gbracha.

@kasperl
Copy link

kasperl commented Sep 3, 2012

cc @floitschG.
Set owner to @kasperl.
Added this to the M1 milestone.

@lrhn
Copy link
Member

lrhn commented Sep 25, 2012

While I would like identical on numbers (and strings) to be bitwise equality, not reference equality, it isn't. Dart2js is still wrong here - identical-tests on the same reference must give true, and the reference in the "nan" variable doesn't change.

@kasperl
Copy link

kasperl commented Oct 8, 2012

Removed this from the M1 milestone.
Added this to the M2 milestone.

@kasperl
Copy link

kasperl commented Oct 17, 2012

Removed this from the M2 milestone.

@kasperl
Copy link

kasperl commented Oct 17, 2012

Added this to the Later milestone.

@kasperl
Copy link

kasperl commented May 23, 2013

Added TriageForM5 label.

@kasperl
Copy link

kasperl commented May 28, 2013

The specification has been updated to treat identical between doubles like this:

The identical() function is the predefined dart function that returns true iff its two arguments are either:
 - The same object.
 - Of type int and have the same numeric value.
 - Of type double, are not NaNs, and have the same numeric value.

As far as I can tell, this works as expected in dart2js, but the VM yields true for the following identical invocation:

   identical(double.NAN, double.NAN)

and should not.


Removed the owner.
Removed this from the Later milestone.
Removed Area-Dart2JS, TriageForM5 labels.
Added Area-VM label.
Changed the title to: "vm: identical(double.NAN, double.NAN) should be false".

@kasperl
Copy link

kasperl commented May 28, 2013

The spec is a little bit unclear on how to deal with identical(double.NAN, double.NAN). Doubles are objects so the first clause in the description of identical means that it should return true and the last one means that it should return false. I think it might be worth calling out what we expect identical(NAN, NAN) to be -- and ideally we'll define it so it doesn't depend on boxing only on the numeric value.


cc @sgmitrovic.
Removed Area-VM label.
Added Area-Language label.
Changed the title to: "identical(double.NAN, double.NAN) should be ?".

@floitschG
Copy link
Contributor

There is also slight ambiguity what "same numeric value" means.
Does -0.0 have the same numeric value as 0.0? They are ==.

Fwiw I think that two doubles should be identical if they have the same bit-pattern. This assumes that NaN always has the same bit-pattern (and the same sign).
Afaics this is how the VM has identity currently implemented.

@gbracha
Copy link
Contributor

gbracha commented May 28, 2013

Re comment 15:

The spec (as listed in comment 14) says "either". That means a disjunction among the 3 clauses. So, if you have a NaN boxed in a given object, it is identical to itself. I think it will be costly to do otherwise, but you can decide that with the VM folks as you wish. All I am saying is that there is no ambiguity between the different clauses.

Re comment 16. I agree we need to clarify the treatment of things like -0.0. My reading would be that -0.0 has the same numeric value as 0.0, assuming numeric values come from the real numbers. But maybe you do want to tell them apart somehow.


Set owner to @gbracha.
Added Accepted label.

@floitschG
Copy link
Contributor

wrt NaN: I don't think we should ever expose that doubles can be boxed. Therefore making boxed NaNs use the identity on their boxes seems wrong. (Nothing prevents NaNs to be in different boxes). But I think I agree with the intention: identical(NaN, NaN) -> true.

wrt -0.0, 0.0:
I would be surprised if identical(a, b), but a.toString() != b.toString()
Since -0.0 and 0.0 print differently I think they should not be identical.

Maybe it would be easier to spec by saying that the library canonicalizes all numbers (ints and doubles). This makes identical non-special and shifts the burden to the library (at least conceptually).

@gbracha
Copy link
Contributor

gbracha commented Jul 2, 2013

Section 12.0.1 of the 0.51 spec deals with this.


Added Done label.

copybara-service bot pushed a commit that referenced this issue May 20, 2022
Changes:
```
> git log --format="%C(auto) %h %s" c1eb6cb..b149f80
 https://dart.googlesource.com/protobuf.git/+/b149f80 Remove the top level analysis_options.yaml file (#656)
 https://dart.googlesource.com/protobuf.git/+/8f6f307 Fix comment syntax
 https://dart.googlesource.com/protobuf.git/+/665f7b0 Remove trailing whitespace in protobuf/LICENSE
 https://dart.googlesource.com/protobuf.git/+/9ffbaf2 Fix default list type for frozen messages (#654)
 https://dart.googlesource.com/protobuf.git/+/a68bb5a Fix Fixed32 to be parsed as unsigned when parsing proto3 protos (#655)
 https://dart.googlesource.com/protobuf.git/+/6957c98 Convert the field started with underscore and digit to a legal name (#651)
 https://dart.googlesource.com/protobuf.git/+/71defca Account for double.infinity and double.nan in json serializers (#652)
 https://dart.googlesource.com/protobuf.git/+/5a48349 Allow `Timestamp.toDateTime()` to return a `DateTime` in local timezone (#653)
 https://dart.googlesource.com/protobuf.git/+/117e869 Make the toString of enums be the value if names are omitted (#649)
 https://dart.googlesource.com/protobuf.git/+/88c4016 Align the hashCode of a message with an empty unknown field-set with that of no unknown field set (#648)
 https://dart.googlesource.com/protobuf.git/+/eed09c4 Fix proto3 repeated field encoding without packed option (#635)
 https://dart.googlesource.com/protobuf.git/+/8f587b1 Simplify `_FieldSet` getters (#646)
 https://dart.googlesource.com/protobuf.git/+/494f189 Fix compile-protos.sh interpreter (#645)
 https://dart.googlesource.com/protobuf.git/+/5e1a422 Fix typo in protobuf-0.9.0+1 changelog
 https://dart.googlesource.com/protobuf.git/+/46df68a Add `UseResult` annotation to `rebuild` (#631)
 https://dart.googlesource.com/protobuf.git/+/ff5304f Migrate protoc_plugin to null safety (#642)
 https://dart.googlesource.com/protobuf.git/+/657197d Fix typo in GeneratedMessage.copyWith documentation (#641)
 https://dart.googlesource.com/protobuf.git/+/e30a522 Fix sharing coded buffer bytes when parsing `bytes` fields (#640)
 https://dart.googlesource.com/protobuf.git/+/810b166 Clear unknown field when setting an extension field with the same tag (#639)
 https://dart.googlesource.com/protobuf.git/+/d30623b Treat empty and uninitialized Maps the same in equality checks (#638)
 https://dart.googlesource.com/protobuf.git/+/4fe3ee4 Make `MapFieldInfo` key and value types non-nullable (#600)
 https://dart.googlesource.com/protobuf.git/+/c26ac34 Add grpc example to protoc_plugin README (#514)
 https://dart.googlesource.com/protobuf.git/+/c35d787 Revert changes to reserved names to maintain backwards compat (#636)
 https://dart.googlesource.com/protobuf.git/+/146b186 Remove unused `GeneratedMessage` constructors (#634)
 https://dart.googlesource.com/protobuf.git/+/1b12ac9 Remove a closure in `_FieldSet.hashCode` (#633)
 https://dart.googlesource.com/protobuf.git/+/5731242 Minor fix in query_bench set_nested_value benchmark: (#630)
 https://dart.googlesource.com/protobuf.git/+/767ce81 Remove fold() closures from `FieldSet._hashCode`. (#554)
 https://dart.googlesource.com/protobuf.git/+/99bc541 protoc_plugin README fixes and tweaks: (#617)
 https://dart.googlesource.com/protobuf.git/+/e282e17 protobuf benchs: invoke protoc once with all protos (#623)
 https://dart.googlesource.com/protobuf.git/+/bef672b protobuf benchs: fix old --trust-type-annotations flag (#622)
 https://dart.googlesource.com/protobuf.git/+/d072e5f dependabot: check for updates monthly (#620)
 https://dart.googlesource.com/protobuf.git/+/96bdf38 Fix TypeRegistry passing when unpacking nested Any messages from JSON (#568)
 https://dart.googlesource.com/protobuf.git/+/4ec722a Correctly combine hash codes for repeated enums. (#556)
 https://dart.googlesource.com/protobuf.git/+/b96dc21 Testing: don't return static methods in findMemberNames (#618)
 https://dart.googlesource.com/protobuf.git/+/09e8a8d Update protobuf/benchmarks README for #553
 https://dart.googlesource.com/protobuf.git/+/3ef4539 Add a benchmark for computing hashCodes. (#553)
 https://dart.googlesource.com/protobuf.git/+/d232e6e Tweak READMEs: (#610)
 https://dart.googlesource.com/protobuf.git/+/0f01fa9 Update protobuf pubspec.yaml (#616)
 https://dart.googlesource.com/protobuf.git/+/a10426b Update protoc_plugin pubspec.yaml (#615)
 https://dart.googlesource.com/protobuf.git/+/a0021c7 Make `protoName` unCamelCase lazy (#606)
 https://dart.googlesource.com/protobuf.git/+/ded1ac7 Document map key and value fields (#603)
 https://dart.googlesource.com/protobuf.git/+/8792f2a Remove redundant check in PbMap equality check (#604)
 https://dart.googlesource.com/protobuf.git/+/b7f9569 Tweak BuilderInfo and FieldInfo docs: (#597)
 https://dart.googlesource.com/protobuf.git/+/6f85c32 Remove a redundant cast (#598)
 https://dart.googlesource.com/protobuf.git/+/3df8669 Latest mono_repo (#601)
 https://dart.googlesource.com/protobuf.git/+/9da84ae Fix a potential issue in CodedBufferWriter (#594)
 https://dart.googlesource.com/protobuf.git/+/6be405f Remove old and unused test file (#589)
 https://dart.googlesource.com/protobuf.git/+/900cef5 Fix protoc_plugin run-tests make rule (#586)
 https://dart.googlesource.com/protobuf.git/+/2546269 Fix rounding when handling negative timestamps (#580)
 https://dart.googlesource.com/protobuf.git/+/8afce8d Fix Readme `pub get` instead of `pub install`. (#486)
 https://dart.googlesource.com/protobuf.git/+/782fd24 Avoid runtime function type check in lazily created singleton creator functions (#574)
 https://dart.googlesource.com/protobuf.git/+/a7e75cb Update README.md
 https://dart.googlesource.com/protobuf.git/+/23136dc Version bump (#563)
 https://dart.googlesource.com/protobuf.git/+/18346f5 Convert null keys and values to default when parsing map entries (#536)
 https://dart.googlesource.com/protobuf.git/+/bb4cf0b Bump to latest mono_repo - use latest actions (#561)
 https://dart.googlesource.com/protobuf.git/+/835ab75 Remove unneeded imports
 https://dart.googlesource.com/protobuf.git/+/ef733ac latest mono_repo
 https://dart.googlesource.com/protobuf.git/+/ecfb862 Fix test for latest analysis (#543)
 https://dart.googlesource.com/protobuf.git/+/26a0a26 fix insecure link in protoc_plugin readme
 https://dart.googlesource.com/protobuf.git/+/444e855 Bump dart-lang/setup-dart from 1.1 to 1.2 (#535)
 https://dart.googlesource.com/protobuf.git/+/d3e0e4a Fix analysis options
 https://dart.googlesource.com/protobuf.git/+/cf29280 Fix comment references

```

Diff: https://dart.googlesource.com/protobuf.git/+/c1eb6cb51af39ccbaa1a8e19349546586a5c8e31~..b149f801cf7a5e959cf1dbf72d61068ac275f24b/

Tested: relying on existing SDK and protobuf tests
Change-Id: I7f89b998a0aba13999d180ee9814a26a5f1d054d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/245228
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Ömer Ağacan <omersa@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).
Projects
None yet
Development

No branches or pull requests

7 participants