Skip to content

Fix void as a type parameter by rewriting most of element_type.dart #1629

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 18 commits into from
Mar 13, 2018

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Mar 8, 2018

Fixes #1625. As mentioned in that bug, the amount of "if (type) do_thing;" spread throughout ElementType (not to mention the different meanings of its methods in different contexts) was making it too hard to rationalize about the intended behavior to try to extend it again for void as a type parameter.

Instead, I've refactored and split ElementType into a set of subclasses and got almost all the if type stuff moved into a factory constructor. This should result in more-or-less identical behavior to the old version, except now, Future<void> and anything else with a void type parameter should function correctly. The subclasses are roughly paralleling the class structure of DartType in the analyzer near the root, with changes appropriate to dartdoc's needs. It's not perfect, but perfect is the enemy of good and I wanted to get this out as quickly as I could.

One minor behavior change is that code that didn't specify a type at all for something where there's no type inference will be documented as "dynamic" rather than having no listed type.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Mar 8, 2018
@devoncarew
Copy link
Member

Is this ready for review?

@jcollins-g
Copy link
Contributor Author

I think it's ready for a start. There's a niggling regression on Flutter I haven't quite ironed out yet.

@jcollins-g
Copy link
Contributor Author

This should be ready for a serious look now. Added @bwilkerson in case he'd like to comment on the new object model. Definitely open to suggestions though I might defer some of them; I sadly had to do this redesign in a hurry for the P0.

@jcollins-g
Copy link
Contributor Author

@kevmoo I'd like your input on the impact of sprinkling dynamic in more places; specifically, in (non-type) parameters. It's particularly noticeable in operator== inherited from Object.

screenshot from 2018-03-12 10-59-08

I introduced it by accident in the element_type rewrite and I can find a way to get rid of that, but I actually find the docs more readable this way. It's clear then that this is a dynamic parameter rather than some other inferred type (which if you're looking at the code it can sometimes be hard to tell).

@kevmoo
Copy link
Member

kevmoo commented Mar 12, 2018

I think it's okay...do you have other examples?

@jcollins-g
Copy link
Contributor Author

jcollins-g commented Mar 12, 2018

@kevmoo:

We now show it as a return-type from a function parameter (edited to have correct screenshot)

screenshot from 2018-03-12 11-23-48

And, as a type for a parameter to an optional function parameter:

screenshot from 2018-03-12 11-28-26

@kevmoo
Copy link
Member

kevmoo commented Mar 12, 2018 via email

@jcollins-g
Copy link
Contributor Author

will do :-)

@jcollins-g jcollins-g merged commit 5dc7270 into master Mar 13, 2018
@jcollins-g jcollins-g deleted the future-void branch March 13, 2018 15:45
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.

4 participants