Issue 2867493002: fix #27256, track type bounds for generic functions

Issue 2867493002: fix #27256, track type bounds for generic functions (Closed)

Created:
3 years, 7 months ago by Jennifer Messerly

Modified:
3 years, 7 months ago

Reviewers:
vsm

CC:
dev-compiler+reviews_dartlang.org

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

fix #27256, track type bounds for generic functions R=vsm@google.com Committed: https://github.com/dart-lang/sdk/commit/b58f826976f5fdca32e20d9dec23e0cad7721d3a

Patch Set 3 : fix #

Created: 3 years, 7 months ago

(Patch set is too large to download)

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+580 lines, -392 lines)			Patch
M	pkg/dev_compiler/lib/sdk/ddc_sdk.sum	View	1 2	Binary file		0 comments	Download
M	pkg/dev_compiler/lib/src/compiler/code_generator.dart	View	1 2	2 chunks	+40 lines, -49 lines	0 comments	Download
M	pkg/dev_compiler/test/browser/runtime_tests.js	View	1 2	25 chunks	+223 lines, -223 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/BenchmarkBase.js	View	1 2	3 chunks	+12 lines, -12 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/async_helper.js	View	1 2	1 chunk	+7 lines, -7 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/closure.js	View	1 2	2 chunks	+20 lines, -20 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/destructuring.js	View	1 2	1 chunk	+8 lines, -8 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/es6_modules.js	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/map_keys.js	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/node_modules.js	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/script.js	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/sunflower/sunflower.js	View	1 2	2 chunks	+4 lines, -4 lines	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/varargs.js	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/classes.dart	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart	View	1 2	2 chunks	+1 line, -4 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/rtti.dart	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart	View	1 2	9 chunks	+179 lines, -49 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/js_mirrors.dart	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
A	tests/language_strong/generic_function_bounds_test.dart	View	1 2	1 chunk	+70 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

vsm

lgtm https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/lib/src/compiler/code_generator.dart File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/lib/src/compiler/code_generator.dart#newcode3038 pkg/dev_compiler/lib/src/compiler/code_generator.dart:3038: if (!typeFormalTypes.every((t) => t.bound.isDynamic)) { Should this be ...

3 years, 7 months ago (2017-05-05 23:49:25 UTC) #3

Jennifer Messerly

Thank you! Addressed those and landing :) https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/lib/src/compiler/code_generator.dart File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right): https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/lib/src/compiler/code_generator.dart#newcode3038 pkg/dev_compiler/lib/src/compiler/code_generator.dart:3038: if (!typeFormalTypes.every((t) ...

3 years, 7 months ago (2017-05-09 18:42:26 UTC) #4

Thank you! Addressed those and landing :)

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3038: if
(!typeFormalTypes.every((t) => t.bound.isDynamic)) {
On 2017/05/05 23:49:25, vsm wrote:
> Should this be isDynamic || isObject?

oh wow, very interesting point. I was using `dynamic` because that's what
Analyzer does for the common case of <T> with no bound. However, it looks like
Analyzer should actually be using `Object` under the current spec (unlike what
InstantiateToBounds does, or the --no-implicit-dynamic flags).

What I'll do instead is check the TypeParameterElement and see if `bound` is
null. That's closer to what this is trying to do.

(Ultimately it's just an optimization, so we're emitting less data for the very
common case of no bounds)

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:588: // then the
whole type is malbounded and an error should be reported
On 2017/05/05 23:49:25, vsm wrote:
> Can we actually hit this at runtime?  Or is a dcall path?

I think so, yup. Via dcall. Adding a test for it.

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:618: /// optional
list of upper bound constraints for each the type formals.
On 2017/05/05 23:49:24, vsm wrote:
> Optional list or optional bounds fn?  Code above seems to expect a fn.

oops. yes that comment is old. will fix. also good catch!

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:909: //
instantiated with the same ones. The instantiate operation is guarenteed
On 2017/05/05 23:49:24, vsm wrote:
> guarenteed -> guaranteed

Done.

https://codereview.chromium.org/2867493002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:910: // to avoid
capture because it does not depend on its TypeVaraible objects,
On 2017/05/05 23:49:25, vsm wrote:
> TypeVaraible -> TypeVariable

Done.

Jennifer Messerly

3 years, 7 months ago (2017-05-09 18:42:50 UTC) #6

Message was sent while issue was closed.

Committed patchset #3 (id:20001) manually as
b58f826976f5fdca32e20d9dec23e0cad7721d3a.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages