Skip to content

Regression in function type checking #25743

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
vsmenon opened this issue Feb 10, 2016 · 9 comments
Closed

Regression in function type checking #25743

vsmenon opened this issue Feb 10, 2016 · 9 comments
Labels
closed-duplicate Closed in favor of an existing report legacy-area-analyzer Use area-devexp instead.

Comments

@vsmenon
Copy link
Member

vsmenon commented Feb 10, 2016

The following code used to work fine (no strong mode errors).

void foo(x) {
  print(x);
}

void bar(void f(int x), int x) {
  f(x);
}

void main() {
  bar(foo, 42);
}

It now gives a static error:

error] Type check failed: foo ((dynamic) → void) is not of type (int) → void because foo cannot be typed as (int) → void

I believe we used to treat foo as top -> void?

@leafpetersen

@vsmenon
Copy link
Member Author

vsmenon commented Feb 10, 2016

This is with analyzer tip. I don't see an error in 1.14.0-dev.5.0.

@jmesserly
Copy link

Let me try that as well after https://codereview.chromium.org/1678313002/ ... we were dropping the fuzzy arrows flag in a few places.

@jmesserly
Copy link

Oh, nvm, my CL only affects generic functions.

I thought we interpreted foo as ⊥ -> void when we're doing subtype checks between two function types?

@vsmenon
Copy link
Member Author

vsmenon commented Feb 10, 2016

I believe @leafpetersen had some logic to handle references to top-level functions - and treat the param type as top.

@jmesserly
Copy link

Oh, hmmmm. I haven't seen any code like that, but that would work.

Vijay, could this regression be caused by our change to get rid of UninferredClosure? We were messing with DownCast.create ... specifically making top level functions fall into an "error" path :)

@jmesserly
Copy link

But whatever it was, none of our checker tests caught it, so I'm not sure...

@jmesserly
Copy link

Actually, isn't this a dupe of #24507?
I'm going to merge it in.

@jmesserly jmesserly added the closed-duplicate Closed in favor of an existing report label Feb 10, 2016
@vsmenon
Copy link
Member Author

vsmenon commented Feb 10, 2016

Yeah, I think it's the same - thanks for merging. I think the "regression" is really a warning changing to an error - which is exactly what you're saying. :-)

Note, this is causing a break of sorts when I run ddc presubmit tests against analyzer tip, but only because one of the async tests we explicitly run at the end of browser_tests is no longer generated (due to warning promoted to error and no force compile I believe).

@jmesserly
Copy link

Note, this is causing a break of sorts when I run ddc presubmit tests against analyzer tip, but only because one of the async tests we explicitly run at the end of browser_tests is no longer generated (due to warning promoted to error and no force compile I believe).

DOH!

In the short term maybe work around it in the test? And I also bumped the priority of #24507. If we do want to special case original function declarations, we can certainly do that in DownCast.create.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-duplicate Closed in favor of an existing report legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

No branches or pull requests

2 participants