Skip to content

Fix multiple issues with properties and top level variables #1466

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

Merged
merged 27 commits into from
Jul 6, 2017

Conversation

jcollins-g
Copy link
Contributor

Fixes #1394. @Hixie Splits the Accessor class into InheritableAccessor (for Class) and Accessor (used directly for TopLevelVariable), rewiring various parts of the object model and factories to better suit the new structure. This includes generating Fields/TopLevelVariables from Accessors rather than the other way around.

This doesn't fix everything for fields (TODOs added noting possible problem areas), but fixing any other problems should be substantially easier with this refactor.

Of particular note is the workaround for FieldElement only containing the current class's explicit getter in a case of an inherited, explicit setter (or vice versa), here. If there's no good reason why the FieldElement from analyzer fails to contain that, perhaps we can change it. @bwilkerson ?

Another similar problem is worked around here -- two different synthetic FieldElements in a weird inheritance corner case where Dartdoc has to pick one to represent the field. The hack here is somewhat arbitrary but wouldn't be necessary if synthetic FieldElements took inheritance into account.

Browseable examples:
old:
http://jcollins1.pdx.corp.google.com:8013/flutter/painting/FlutterLogoDecoration-class.html
new: (path change is due to how I built this as part of my testing)
http://jcollins1.pdx.corp.google.com:8014/painting/FlutterLogoDecoration-class.html

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Jun 22, 2017
@jcollins-g jcollins-g changed the title Field fix Fix multiple issues with properties and fields Jun 22, 2017
@jcollins-g jcollins-g requested review from bwilkerson and keertip June 22, 2017 19:30
@jcollins-g jcollins-g changed the title Fix multiple issues with properties and fields Fix multiple issues with properties and top level variables Jun 22, 2017
@bwilkerson
Copy link
Member

Of particular note is the workaround for FieldElement only containing the current class's explicit getter in a case of an inherited, explicit setter (or vice versa), here. If there's no good reason why the FieldElement from analyzer fails to contain that, perhaps we can change it.

The primary reason for the current implementation, as unfortunate as it is, has to do with parenting. Consider the following example:

class A {
  void set x(int x) ...
}
class B extends A {
  int get x ...
}

If we created a single FieldElement to represent the (x, x=) pair, which class would we choose to be the parent? Either way, one of the property accessors appears to be defined in the wrong class. And if the classes are in different compilation units or libraries, then the problem only gets worse.

I honestly think the right solution is to not create synthetic FieldElements at all and to provide an API that will do the appropriate searching when we need to locate the "paired" accessor. But that's a bigger change than I can take on right now (and one that might be impacted by the move to the new front end).

@jcollins-g
Copy link
Contributor Author

That's quite reasonable -- and I think your proposed future direction is in line with the direction dartdoc took for this problem (deriving Fields from Accessors). In dartdoc, we have that extra layer of abstraction that at least enables us to paper over it for now -- each analyzer element can have different enclosing classes depending on where we are looking from -- so we don't run into a hard wall.

factory ModelElement.from(Element e, Library library,
{Class enclosingClass, int index, ModelElement enclosingCombo}) {
{Class enclosingClass, int index, Accessor getter, Accessor setter}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea: Some of the code might be cleaner if you allowed e to be null and simply returned null. It would eliminate the need for a fair number of null checks. It's also consistent with some of the other factories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO added, will try for this in a followup. To really do this right, the library construction also has to change to allow for this too as most places where we're calling ModelElement.from from also construct the library.

@override
bool get hasSetter => _variable.setter != null;
//@override
//bool get hasSetter => _variable.setter != null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the commented out code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jcollins-g jcollins-g merged commit f480e0b into master Jul 6, 2017
@kevmoo kevmoo deleted the field-fix branch July 7, 2017 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FlutterLogoDecoration.isComplex isn't showing its documentation
3 participants