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

absent optional parameter for method seemingly not passed through as 'null' #2074

Closed
DartBot opened this issue Mar 9, 2012 · 18 comments
Closed

Comments

@DartBot
Copy link

DartBot commented Mar 9, 2012

This issue was originally filed by ross.m.s...@gmail.com


Based on the forum thread: https://groups.google.com/a/dartlang.org/group/misc/browse_thread/thread/08270b5e76b467ae#

this will print 'NULL' but not stroke my rect:

void strokeRectangle(Rectangle r, [num lineWidth]) {
    if(lineWidth == null) print("NULL");
    _context2d.strokeRect(r.left, r.top, r.width, r.height, lineWidth);
  }

this will stroke my rect:

void strokeRectangle(Rectangle r, [num lineWidth]) {
    if(lineWidth == null) print("NULL");
    _context2d.strokeRect(r.left, r.top, r.width, r.height, null);
  }

What version of the product are you using? On what operating system?

Dart Editor build 5205 on x86_64

Please provide any additional information below.

"My uninformed guess is you're running into the distinction between
undefined and null in the underlying javascript.
Parameters that aren't passed to a JS function get the value
undefined, which is distinct from null but compares equal to it. "

@kasperl
Copy link

kasperl commented Mar 9, 2012

Thanks a lot for the bug report, Ross!


Set owner to @kasperl.
Removed Priority-Medium label.
Added Priority-High, Area-Frog, Triaged labels.

@kasperl
Copy link

kasperl commented Mar 9, 2012

Yeah, it does look like you're running into the distinction between JS null and undefined. Our compiler currently uses both values to represent Dart null, but sometimes you end up calling external JS code or DOM functions that treat the two JS values differently.

The safest workaround is probably something like this:

  void strokeRectangle(Rectangle r, [num lineWidth]) {
    if (lineWidth == null) {
      _context2d.strokeRect(r.left, r.top, r.width, r.height);
    } else {
      _context2d.strokeRect(r.left, r.top, r.width, r.height, lineWidth);
    }
  }

which is admittedly a bit harder on the eyes.

@DartBot
Copy link
Author

DartBot commented Mar 11, 2012

This comment was originally written by ross.m.smith...@gmail.com


thanks for looking into this Kasper! I have the workaround in place(s) for now :)

@rakudrama
Copy link
Member

This is not a distinction between null and undefined.

strokeRect takes four arguments.

Many JavaScript DOM functions behave differently depending on the number of arguments, for example, passing 5 when only 4 are accepted does not simply ignore the 5th like a JavaScript function would.

Frog uses both undefined and null as Dart 'null'.
As an optimization, it leaves off arguments that are known to be null, since the default 'undefined' is an acceptable null.
So
   _context2d.strokeRect(r.left, r.top, r.width, r.height, null);
compiles as
   _context2d.strokeRect(r.left, r.top, r.width, r.height);

and magically works.

It is unfortunate for Dart that the DOM functions dispatch on the number of arguments.
Sometimes there is no value of y to make f(x, y) behave exactly like f(x).

As mentioned above, trailing null arguments are removed as an optimization. When there are default arguments, these are added first, and then the nulls are removed. This usually 'accidentally' results in the compiled call having the same number of arguments as the source call:

// Dart:
void f(x, [y, z]);
f(1);
// JS:
-> f(1)
-> f(1, null, null) // Add default arguments
-> f(1) // remove nulls

What needs to be fixed is ensure a source call that may end up calling a DOM method compiles to a call or chain of calls that calls the DOM method with exactly the same number of arguments.
The code that adds the defaults is separate to the code the removes the nulls. Both need to be fixed to improve the situation.

@anders-sandholm
Copy link
Contributor

Removed Area-Frog label.
Added Area-Dart2JS, OldAreaFrog labels.

@anders-sandholm
Copy link
Contributor

Removed OldAreaFrog label.
Added FromAreaFrog label.

@kasperl
Copy link

kasperl commented Jun 8, 2012

We should make sure we have a passing test case for this.


Removed Priority-High, FromAreaFrog labels.
Added Priority-Medium label.
Changed the title to: "absent optional parameter for method seemingly not passed through as 'null'".

@DartBot
Copy link
Author

DartBot commented Jun 10, 2012

This comment was originally written by ross.m.s...@gmail.com


Yes, please add test coverage for this :) This issue is still present in build 8365. I get it in dart2js as well as Dartium now, so perhaps the root cause is different than it used to be - but there does still seem to be a bug.

@kasperl
Copy link

kasperl commented Sep 3, 2012

Set owner to ngeoffray@google.com.
Added this to the Later milestone.

@kasperl
Copy link

kasperl commented Oct 17, 2012

Removed this from the Later milestone.

@kasperl
Copy link

kasperl commented Oct 17, 2012

Added this to the M2 milestone.

@DartBot
Copy link
Author

DartBot commented Dec 11, 2012

This comment was originally written by ngeoffray@google.com


Removed this from the M2 milestone.
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

Is this still a problem? Most problems don't go away without us actively fixing them, but I'm not sure if this has gone stale.


Removed TriageForM5 label.
Added AssumedStale label.

@peter-ahe-google
Copy link
Contributor

Kasper, you said: "We should make sure we have a passing test case for this." on Jun 8, 2012.


Added Triaged label.

@DartBot
Copy link
Author

DartBot commented May 30, 2013

This comment was originally written by ross.m....@gmail.com


I did a few quick tests in SDK 0.5.11_r23200 and this issue (forwarding null for absent optional) appears to be fixed; I'd still encourage that test case for regression's sake ;)

In other news passing 'null' as 'lineWidth' to 'strokeRect' works in dart2js but in Dartium it now throws an error:

Exception: Dart_DoubleValue expects argument 'double_obj' to be non-null.

@DartBot
Copy link
Author

DartBot commented Jun 11, 2013

This comment was originally written by ross.m....@gmail.com


Interestingly, the optional [num lineWidth] seems to have been since removed from the CanvasRenderingContext2D spec and from the Blink implementation, so the sample code in this issue is no longer useful , which (I think) explains the observations in my last comment.

As mentioned in my last comment though, I believe the original issue to be fixed now.

cheers,

@kasperl
Copy link

kasperl commented Sep 18, 2013

Marking as fixed based on Ross' comment.


Added Fixed label.

@DartBot DartBot added this to the Later milestone Sep 18, 2013
copybara-service bot pushed a commit that referenced this issue Apr 24, 2023
…n, sse, stream_channel, test, tools, usage, webdev

Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/0127813..ce650b0):
  ce650b0  2023-04-19  Nate Bosch  Regression test for error rejecting transaction (#230)

characters (https://github.com/dart-lang/characters/compare/ba8d557..b306414):
  b306414  2023-04-19  Lasse R.H. Nielsen  Add tools to update and generate tables and tests. (#72)

convert (https://github.com/dart-lang/convert/compare/8812e40..855aeac):
  855aeac  2023-04-10  Kevin Moore  Require Dart 2.19, fix override param name, update lints (#81)

crypto (https://github.com/dart-lang/crypto/compare/1cb1528..77491f5):
  77491f5  2023-04-19  Lasse R.H. Nielsen  Make `DigestSink` implement `Sink` (#146)

dartdoc (https://github.com/dart-lang/dartdoc/compare/a0755f5..8e8b36e):
  8e8b36e3  2023-04-19  Janice Collins  Fix up documentation on comment reference parser to align with wiki (#2851)
  b9178fce  2023-04-19  dependabot[bot]  Bump github/codeql-action from 2.2.11 to 2.2.12 (#3390)
  37b981c4  2023-04-19  dependabot[bot]  Bump actions/checkout from 3.5.0 to 3.5.2 (#3389)
  dadef24a  2023-04-10  dependabot[bot]  Bump github/codeql-action from 2.2.9 to 2.2.11 (#3386)
  dda699a7  2023-04-10  Sam Rawlins  Prepare mixin-like classes for Dart 3.0.0 (#3385)

intl (https://github.com/dart-lang/intl/compare/a958db0..5d65e38):
  5d65e38  2023-04-19  Moritz  Update README.md
  9972a89  2023-04-19  Moritz  Update README.md
  5c14faa  2023-04-18  Copybara-Service  Merge pull request #584 from dart-lang:fixDateFormattingUrl
  4ea644e  2023-04-17  Moritz  Merge branch 'master' into fixDateFormattingUrl
  b0d5687  2023-04-17  dependabot[bot]  Bump dart-lang/setup-dart from 1.4.0 to 1.5.0 (#574)
  92149be  2023-04-17  Moritz  Merge branch 'master' into fixDateFormattingUrl
  5203d6e  2023-04-17  Googler  Internal change
  bab667f  2023-04-17  Moritz  Typo
  b0896b1  2023-04-17  Moritz  Fix bug
  43507e1  2023-04-14  Googler  Internal change
  d8844a0  2023-04-14  dependabot[bot]  Bump actions/checkout from 3.3.0 to 3.5.2 (#579)
  99ed16f  2023-04-13  Copybara-Service  Merge pull request #572 from dart-lang:adaptBrowserTest
  34e824c  2023-03-30  Moritz  Adapt test to browser CLDR update

markdown (https://github.com/dart-lang/markdown/compare/d437c85..5f98aea):
  5f98aea  2023-04-20  Jonas Finnemann Jensen  Throw, if `BlockSyntax.parseLines` loops indefinitely (#533)
  86ebc2c  2023-04-19  Zhiguang Chen  Fix `linkReferenceDefinitionPattern` (#532)

sse (https://github.com/dart-lang/sse/compare/11e83a0..f947c3d):
  f947c3d  2023-04-10  Kevin Moore  Require Dart 2.19, bump lints (#82)

stream_channel (https://github.com/dart-lang/stream_channel/compare/74646ea..71d4690):
  71d4690  2023-04-11  Tobe Osakwe  Add example/example.dart (#52)

test (https://github.com/dart-lang/test/compare/7832931..7fab079):
  7fab0792  2023-04-19  Oleh Prypin  Dart 3 compatibility: change `extends Iterator` to `implements Iterator` (#1997)
  8f7682a5  2023-04-18  Nate Bosch  Remove deprecation of test_api top level libraries (#1994)
  7151486c  2023-04-14  Simon Binder  Add support for Microsoft Edge (#1992)
  c1d686aa  2023-04-12  Parker Lougheed  Fix "Improvements" link in `package:checks` migration guide (#1991)

tools (https://github.com/dart-lang/tools/compare/545d7e1..5c9f45c):
  5c9f45c  2023-04-20  Elias Yishak  Move `dateStamp` getter to `utils.dart` (#83)

usage (https://github.com/dart-lang/usage/compare/0698711..f97752f):
  f97752f  2023-04-10  Devon Carew  update readme for deprecation (#192)

webdev (https://github.com/dart-lang/webdev/compare/7546291..8b42c95):
  8b42c950  2023-04-21  Anna Gringauze  Cleanup record types display (#2070)
  1cfb3bd6  2023-04-20  Elliott Brooks  Update CONTRIBUTING docs (#2097)
  941eda5b  2023-04-19  Elliott Brooks  Add a test to make sure proper release procedure is followed for `dwds` and `webdev` (#2095)
  2eb7c3ee  2023-04-17  Elliott Brooks  Add Github workflow for Dart Code Metrics (#2092)
  79a9bc9b  2023-04-17  Elliott Brooks  Fix DCM analyzer warnings (#2094)
  2a625039  2023-04-14  Elliott Brooks  Add `vm_service` git package dependency override (#2078)
  1fa19603  2023-04-12  Elliott Brooks  Add CI check to remind contributor to update CHANGELOG (#2090)
  c7bb19e1  2023-04-12  Elliott Brooks  Prevent PRs labeled `do-not-submit` from being merged (#2088)
  3781ef9b  2023-04-10  Elliott Brooks  Run mono_repo generate with version 6.5.3 (#2084)
  843890c6  2023-04-10  Anna Gringauze  Refactor record shape processing out of calculating record bound fields (#2074)

Change-Id: I4ce65f9f79d5086c33c575e57eff300216392510
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/297800
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Moritz Sümmermann <mosum@google.com>
Commit-Queue: Moritz Sümmermann <mosum@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants