Skip to content

Implement generic function type syntax in the analyzer #27969

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

Closed
floitschG opened this issue Dec 2, 2016 · 21 comments
Closed

Implement generic function type syntax in the analyzer #27969

floitschG opened this issue Dec 2, 2016 · 21 comments
Assignees
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Milestone

Comments

@floitschG
Copy link
Contributor

This is the analyzer-implementation issue for #27527.

At the moment we only really need the new syntax in typedefs. Eventually, we want to make this syntax usable for fields and locals, but it's not yet as high a priority. If you find the time to make it work there too, please guard it behind a (different) flag.

@floitschG floitschG added the legacy-area-analyzer Use area-devexp instead. label Dec 2, 2016
@floitschG floitschG added this to the 1.50 milestone Dec 2, 2016
@bwilkerson bwilkerson added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug labels Dec 5, 2016
@bwilkerson bwilkerson self-assigned this Dec 7, 2016
@mit-mit
Copy link
Member

mit-mit commented Dec 15, 2016

Moving to 1.22 for implementation, informal spec i linked from the meta-issue #27527

@mit-mit mit-mit modified the milestones: 1.22, 1.50 Dec 15, 2016
@kevmoo
Copy link
Member

kevmoo commented Jan 13, 2017

@bwilkerson is cranking on it

@jmesserly
Copy link

jmesserly commented Jan 24, 2017

@bwilkerson I'm happy to take a look at continuing this one through resolution (I noticed you already landed the parser/AST support?)

@bwilkerson
Copy link
Member

Yes, the parser/AST support is in, but there is a flag in the parser that disables it. We can't re-enable it until 1.22 has built, which will make it hard to test any resolution work.

That said, I'm happy to let you finish the work. I had already started working on resolution when we decided to move the support from 1.22 to 1.23, and I've sent you a CL with what I'd gotten done.

@mit-mit
Copy link
Member

mit-mit commented Feb 1, 2017

The branch is open for 1.23, so would be good to get this one moving again!

@bwilkerson
Copy link
Member

Yep. The next step was committed yesterday, but had to be temporarily reverted because of a problem with the way dartk was being built. I believe that's been fixed, so I'm planning on re-landing it today.

@mit-mit
Copy link
Member

mit-mit commented Mar 15, 2017

@bwilkerson was this landed?

@bwilkerson
Copy link
Member

Unfortunately, no. It is requiring more effort than I had originally anticipated.

@dgrove
Copy link
Contributor

dgrove commented Mar 24, 2017

Any further updates? Is this likely to make it for 1.23?

@bwilkerson
Copy link
Member

I have a CL that I will commit in the morning that gets the shared tests to pass. At that point I'll probably consider this issue to be closed, and we can open individual issues as we discover specific cases that aren't yet working.

@bwilkerson
Copy link
Member

Not all the tests are passing; I missed the ones in language-strong. Will look at those today.

@bwilkerson
Copy link
Member

I was wrong, all of the tests I know of related to generic function types appear to be passing. (The others were related to generic methods and my changes happened to have fixed a couple of those cases.) Hence, I'm going to close this and expect to see other issues opened as specific problems are discovered.

@floitschG
Copy link
Contributor Author

The analyzer still crashes on the generated tests.

Try to patch in https://codereview.chromium.org/2707033004 to have more tests.

@floitschG floitschG reopened this Apr 3, 2017
@bwilkerson
Copy link
Member

Are the generated tests not run on the bots?

@floitschG
Copy link
Contributor Author

Not yet.

@leafpetersen
Copy link
Member

Filed two specific issues here:

#29236
#29237

@bwilkerson
Copy link
Member

When I visit the cl (https://codereview.chromium.org/2707033004) it says "(Patch set is too large to download)". Is there another way for me to get the tests?

@leafpetersen
Copy link
Member

git cl patch 2707033004 seems to work.

@leafpetersen
Copy link
Member

#29360

@mit-mit
Copy link
Member

mit-mit commented Apr 18, 2017

#29360 is being merged. Anything else we need, or can we close this one?

@bwilkerson
Copy link
Member

The analyzer passes all known tests, including the generated tests. I'm going to close this and request that any newly discovered deficiencies of the implementation be opened as new issues and not be assigned to the 1.23 milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants