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

Cannot re-create default values. #8

Closed
sigurdm opened this issue Jun 16, 2015 · 4 comments
Closed

Cannot re-create default values. #8

sigurdm opened this issue Jun 16, 2015 · 4 comments

Comments

@sigurdm
Copy link
Collaborator

sigurdm commented Jun 16, 2015

With the interface the analyzer currently present us it is very difficult to reproduce a constant from the default value to a function parameter in generated code.

For a simplified example, given:

class A {
  A([a = XXXX]);
}

We would like to be able to produce:

newInstance(List arguments) {
  Function x = ([a = YYYY]) => new A(a);
  Function.apply(x, arguments);
}

Such that YYYY has the same value as XXXX.
For example if we have:

class A {
  static const c = 10;
  A([a = c]);
}

then letting YYYY = XXXX will fail.
This might be possible after a resolution of: dart-lang/sdk#17307.
When we have generalized tearoffs: dart-archive/dart_enhancement_proposals#3 this issue might disappear.

@eernstg
Copy link
Collaborator

eernstg commented Jun 19, 2015

Just considering a possible work-around: Is there an easy way to tell how many levels of lifting any particular piece of code will tolerate without getting a different semantics? (by 'lifting' I mean: moving a piece of code from inside to outside a given scope, similar to lambda lifting and similar to loop invariant expression hoisting). For code in a constructor or method argument list default expression, we'd just need to know that we can lift the expression one level (to global), and then we'd control the target context well enough (we're generating it) to make sure the expression doesn't get a different semantics when inserted there. This means that we would be able to use the naive approach for moving the code as long as it works, and then we could bail out with an error message in the cases where something more elaborate is needed (or we could run some other algorithms and cover more and more cases as we proceed, leaving fewer and fewer cases unimplemented).

eernstg added a commit that referenced this issue Aug 20, 2015
Added a test 'implicit_getter_setter_test.dart' which explores the
properties of implicit getters and setters for various kinds of
instance and static variable declarations.

Refactored the representation of mirrors in transformed code, in order
to enable unusual things for this purpose: The parameter of an implicit
setter has a special name, finding the owner of several of the relevant
mirrors used to involve some circularities (that we couldn't see how
to break with the old setup),

Deleted several declarations which are not used because of the
refactoring. Deleted code which used to create implicit accessor
mirrors (actually it was dead code, which was one of the bugs, but
now it's replaced by different code in a different location).

Implemented `staticMembers` on `ClassMirrorImpl` in order to support
static implicit accessors, and added various members earlier in the
pipeline to support it (`_staticMemberCache` and others).

Reclassified 'parameter_mirrors_test.dart' in .status: It is actually
blocked by the same issue #8 as some other tests (moving code for
the value of default arguments).

R=sigurdm@google.com

Review URL: https://codereview.chromium.org/1297503006 .
@eernstg
Copy link
Collaborator

eernstg commented Aug 22, 2015

Closing this issue: Implemented support for a number of special cases (including default value expressions which have no local dependencies such as literals, and expressions which are simple identifiers looked up in the same class or top-level of the same library); missing cases (e.g., using const somePrefix.C() as a default value or using @somePrefix.C() as metadata) will give rise to an error during transformation. We'll need extra tests for the missing cases as well, but this will enable a lot of simple usages of default values and annotations to work, and the remaining cases can be considered as bugs that we will fix along the way.

@eernstg eernstg closed this as completed Aug 22, 2015
@sigurdm sigurdm reopened this Aug 24, 2015
@sigurdm
Copy link
Collaborator Author

sigurdm commented Aug 24, 2015

Reopening, as we do not support all constants yet.

eernstg added a commit that referenced this issue Aug 26, 2015
Added implementation of methods to handle the operation where code
K is modified to K' such that K' has the same semantics in a library
L' as K had in L. It only covers some simple cases (literals, expressions
based on literals, e.g., `"4" + "2"`, and simple identifiers and
constructor invocations). This is, however, enough for the test cases
that we have so far (including 'new_instance_default_values_test.dart'
that we have kept in .status for a while). Hence, this mostly
resolves issue #8.

Generalized `returnType` to handle `dynamic`, just like `type` in variables
and parameters.

At this point, only two tests are disabled via .status.

R=sigurdm@google.com

Review URL: https://codereview.chromium.org/1314473002 .
@sigurdm
Copy link
Collaborator Author

sigurdm commented Oct 5, 2015

I am convinced now that except for private constants (and that's a different issue) we can reconstruct all constants.

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

No branches or pull requests

2 participants