Add analysis option that will be used to fix #26583

Add analysis option that will be used to fix #26583

There's follow up work that is needed for CLI and analysis_options

R=brianwilkerson@google.com, leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/67dd48060b127ebbaf67b091a2802bc728db3b50

[Jennifer Messerly, 2016-06-08 17:40:06]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2016-06-08 17:48:34]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Jennifer Messerly, 2016-06-08 17:48:35]

The user experience goal is to expose a simple option to make all implicit casts
into warnings:

    dartanalyzer --strong --no-implicit-casts

and a similar thing in analysis_options

I'm not 100% sure this is the right implementation approach, though. It occurred
to me after I finished that there are also ways to post-process the error
stream.

This change also cleans up some of the old "info.dart" code that isn't needed.
That was for DDC errors before it was integrated with Analyzer. I tried to clean
it up more, but DDC depends on the CoercionInfo type still, so we probably want
to stage it somehow (e.g. introduce a new way for DDC to find casts, wean DDC
off CoercionInfo, then after a release we can remove it).

FYI, the bug is here: https://github.com/dart-lang/sdk/issues/26583, but this is
a pretty frequent request from internal users as well (IIRC from a chat with
Leaf)

[Jennifer Messerly, 2016-06-08 17:50:47]

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/info.dart (right):

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/info.dart:397: 'STRONG_MODE_ASSIGNMENT_CAST',
do we need all of these? I started to consolidate them but then backed off, I'm
not sure if anyone is using these to filter errors or not.

ASSIGNMENT_CAST in particular seems like it should be folded into
DOWN_CAST_IMPLICIT. We also discussed (in the github issue) folding in
DYNAMIC_CAST.

[Brian Wilkerson, 2016-06-08 18:31:35]

lgtm

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/info.dart (right):

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/info.dart:146: ErrorCode errorCode;
Creating a new ErrorCode for each error is really weird. I suppose it works, but
I'd like to change this code later to work the way the rest of the framework
works (or understand why it can't if that's the case).

[Jennifer Messerly, 2016-06-08 19:01:52]

Thanks so much Brian!

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/info.dart (right):

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/info.dart:146: ErrorCode errorCode;
On 2016/06/08 18:31:35, Brian Wilkerson wrote:
> Creating a new ErrorCode for each error is really weird. I suppose it works,
but
> I'd like to change this code later to work the way the rest of the framework
> works (or understand why it can't if that's the case).

You're 100% right! It is super weird. I can't think of any reason for it. I
think it dates back to before this code was integrated into Analyzer. That's the
only thing I can think of.

I can take a stab at cleaning it up. I actually have a branch that moves
implicit cast over into StaticWarningCode/HintCode in the normal way ... but in
that branch I also got rid of all the different classifications (they all have
the same error message string) and it wasn't clear to me if the distinctions
actually mattered or not. I guess I could create a suite of ErrorCodes with the
same messages.

[Brian Wilkerson, 2016-06-08 19:06:07]

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/info.dart (right):

https://codereview.chromium.org/2054443002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/info.dart:146: ErrorCode errorCode;
Having multiple error codes would be useful if (a) we think we might want to
differentiate the error (or correction) messages to improve the UX or (b) we
think users might want to ignore / disable errors in some cases but not others.

Cleaning it up would be great, but it might not be our top priority at the
moment.

[Leaf, 2016-06-08 20:10:49]

A couple of high level comments.

1) Is there a benefit to making this a warning instead of an error?  My first
instinct is to say that if someone enables this flag, then they probably want to
treat this as an error anyway.  I guess maybe they could choose to run and then
fix the downcasts later, but... my inclination would be to just make this an
error.

2) I think an alternative implementation approach would be to do this in the
analyzer proper (rather than in the strong mode checker). I think you would have
to either push the analysis options into typeSystem so that the behavior of
isAssignable changed depending on whether or not this flag was set, or else
factor out a set of utility operations that checked the flag before calling
isAssignable.  A complication here is if you want to do different things for
assignments vs function arguments then you would need to get that information to
the right place.  Long term, I think we want to move more functionality away
from the strong mode checker, so it might be worth looking to see if this is
feasible.  But I'm fine with landing this as is if it seems better not to start
down the rabbit hole.

[Jennifer Messerly, 2016-06-08 20:32:47]

On 2016/06/08 20:10:49, Leaf wrote:
> A couple of high level comments.
> 
> 1) Is there a benefit to making this a warning instead of an error?  My first
> instinct is to say that if someone enables this flag, then they probably want
to
> treat this as an error anyway.  I guess maybe they could choose to run and
then
> fix the downcasts later, but... my inclination would be to just make this an
> error.

Yeah I wondered that too :) ... was kind of thinking a warning because we can
still compile/run code with it. And felt a bit more typical for compiler flags
to just add warnings (vs errors). But yeah, definitely could make it an error
too. Was a tossup in my brain. Either way, let me know. :)

> 
> 2) I think an alternative implementation approach would be to do this in the
> analyzer proper (rather than in the strong mode checker). I think you would
have
> to either push the analysis options into typeSystem so that the behavior of
> isAssignable changed depending on whether or not this flag was set, or else
> factor out a set of utility operations that checked the flag before calling
> isAssignable.  A complication here is if you want to do different things for
> assignments vs function arguments then you would need to get that information
to
> the right place.  Long term, I think we want to move more functionality away
> from the strong mode checker, so it might be worth looking to see if this is
> feasible.  But I'm fine with landing this as is if it seems better not to
start
> down the rabbit hole.

Yeah for sure, there's the full integration that we need to do to merge in
CodeChecker. That goes along with Brian's comment too (why do we have another
thing that is almost but not quite an ErrorCode?). I retreated from all my
refactoring attempts after it seemed a bit more gnarly than I was ready to
tackle :). But I really want to get there. I see it as a bit of a multi-step
process:

- introduce some new AST properties for DDC to use to find coecions
- get DDC to stop using CoercionInfo and use the new properties
- introduce ErrorCodes and remove CoercionInfos
- migrate the downcast checking into resolver and/or error verifier
- migrate override checking somewhere (?)
- migrate dynamic call marking somewhere (? perhaps back into DDC)
- things are finally same again. celebrate \o/ !!!

[Jennifer Messerly, 2016-06-08 20:33:12]

On 2016/06/08 20:32:47, John Messerly wrote:
> On 2016/06/08 20:10:49, Leaf wrote:
> > A couple of high level comments.
> > 
> > 1) Is there a benefit to making this a warning instead of an error?  My
first
> > instinct is to say that if someone enables this flag, then they probably
want
> to
> > treat this as an error anyway.  I guess maybe they could choose to run and
> then
> > fix the downcasts later, but... my inclination would be to just make this an
> > error.
> 
> Yeah I wondered that too :) ... was kind of thinking a warning because we can
> still compile/run code with it. And felt a bit more typical for compiler flags
> to just add warnings (vs errors). But yeah, definitely could make it an error
> too. Was a tossup in my brain. Either way, let me know. :)
> 
> > 
> > 2) I think an alternative implementation approach would be to do this in the
> > analyzer proper (rather than in the strong mode checker). I think you would
> have
> > to either push the analysis options into typeSystem so that the behavior of
> > isAssignable changed depending on whether or not this flag was set, or else
> > factor out a set of utility operations that checked the flag before calling
> > isAssignable.  A complication here is if you want to do different things for
> > assignments vs function arguments then you would need to get that
information
> to
> > the right place.  Long term, I think we want to move more functionality away
> > from the strong mode checker, so it might be worth looking to see if this is
> > feasible.  But I'm fine with landing this as is if it seems better not to
> start
> > down the rabbit hole.
> 
> Yeah for sure, there's the full integration that we need to do to merge in
> CodeChecker. That goes along with Brian's comment too (why do we have another
> thing that is almost but not quite an ErrorCode?). I retreated from all my
> refactoring attempts after it seemed a bit more gnarly than I was ready to
> tackle :). But I really want to get there. I see it as a bit of a multi-step
> process:
> 
> - introduce some new AST properties for DDC to use to find coecions
> - get DDC to stop using CoercionInfo and use the new properties
> - introduce ErrorCodes and remove CoercionInfos
> - migrate the downcast checking into resolver and/or error verifier
> - migrate override checking somewhere (?)
> - migrate dynamic call marking somewhere (? perhaps back into DDC)
> - things are finally same again. celebrate \o/ !!!

s/finally same/finally saNe :)

[Jennifer Messerly, 2016-06-09 00:33:02]

Here's another question for you guys ... we currently use the expando AST
property map to mark implicit conversions. That seems slow/memory intensive (a
HashMap allocation which is at least another Array internally, not to mention
our CoercionInfo object). I was thinking just a `HashMap<AstNode, DartType>`
could track the coercions for the entire CompilationUnit (bonus: if there are
none, we can skip an entire tree copy in DDC)

[Jennifer Messerly, 2016-06-09 00:50:40]

BTW, while some of these questions are pending I am building on this CL with
some follow up ones to do more of refactoring (described further up in the
thread).

[Brian Wilkerson, 2016-06-09 13:57:51]

> I was thinking just a `HashMap<AstNode, DartType>` could track the coercions
for the entire CompilationUnit ...

How often does code contain these coersions? If it's 0-2 nodes in most
compilation units then this isn't really an issue. If it's frequent, then it
might be worth optimizing.

If it is worth optimizing, then I like the idea of putting the data on the
CompilationUnit so that clients don't have to pass around multiple data
structures.

> bonus: if there are none, we can skip an entire tree copy in DDC

Seems like the code to compute where these coersions are could also keep track
of whether any were found, making it easy to know this information no matter how
we represent it.

[Leaf, 2016-06-09 22:14:55]

> Yeah I wondered that too :) ... was kind of thinking a warning because we can
> still compile/run code with it. And felt a bit more typical for compiler flags
> to just add warnings (vs errors). But yeah, definitely could make it an error
> too. Was a tossup in my brain. Either way, let me know. :)

I'd say make it an error.

> > 2) I think an alternative implementation approach would be to do this in the
> > analyzer proper (rather than in the strong mode checker). I think you would
> have
> > to either push the analysis options into typeSystem so that the behavior of
> > isAssignable changed depending on whether or not this flag was set, or else
> > factor out a set of utility operations that checked the flag before calling
> > isAssignable.  A complication here is if you want to do different things for
> > assignments vs function arguments then you would need to get that
information
> to
> > the right place.  Long term, I think we want to move more functionality away
> > from the strong mode checker, so it might be worth looking to see if this is
> > feasible.  But I'm fine with landing this as is if it seems better not to
> start
> > down the rabbit hole.
> 
> Yeah for sure, there's the full integration that we need to do to merge in
> CodeChecker. That goes along with Brian's comment too (why do we have another
> thing that is almost but not quite an ErrorCode?). I retreated from all my
> refactoring attempts after it seemed a bit more gnarly than I was ready to
> tackle :). But I really want to get there. I see it as a bit of a multi-step
> process:
> 
> - introduce some new AST properties for DDC to use to find coecions
> - get DDC to stop using CoercionInfo and use the new properties
> - introduce ErrorCodes and remove CoercionInfos
> - migrate the downcast checking into resolver and/or error verifier
> - migrate override checking somewhere (?)
> - migrate dynamic call marking somewhere (? perhaps back into DDC)
> - things are finally same again. celebrate \o/ !!!

I'm not sure I understand why all of these things would be on the implementation
path to doing this in the error-verifier/resolver.  My thinking (quite possibly
wrong) was that if you just changed the definition and/or uses of isAssignable
to disable the additional backwards subtype checks when the --no-implicit-casts
flag was set, then all implicit cast locations would just show up as type
errors.  The backend could keep generating coercion infos etc the same as it
does now (maybe to be re-factored later).  Does this not work?  Or is it just
that this would be detour from where you want to get to?

[Leaf, 2016-06-09 22:16:56]

On 2016/06/09 00:33:02, John Messerly wrote:
> Here's another question for you guys ... we currently use the expando AST
> property map to mark implicit conversions. That seems slow/memory intensive (a
> HashMap allocation which is at least another Array internally, not to mention
> our CoercionInfo object). I was thinking just a `HashMap<AstNode, DartType>`
> could track the coercions for the entire CompilationUnit (bonus: if there are
> none, we can skip an entire tree copy in DDC)

If we can, my inclination is to pull it out of band.  I had the same thought
about the use of the AST property for downwards inference in the resolver: I'm
not sure if it actually matters, but it seems like it would be trivial to move
that over to a hash map in the inference context, and then not have all of these
small hashmaps attached to ast nodes during resolving.

[Brian Wilkerson, 2016-06-09 22:24:08]

> If we can, my inclination is to pull it out of band.  I had the same thought
> about the use of the AST property for downwards inference in the resolver: I'm
> not sure if it actually matters, but it seems like it would be trivial to move
> that over to a hash map in the inference context, and then not have all of
these
> small hashmaps attached to ast nodes during resolving.

One question to ask is whether this information would be useful for other tools.
Would a clean-up tool (such as quick assists) like to be able to insert explicit
casts in places where there are implicit casts? If so, then this information
needs to be part of the public API. Storing the information in a map in the
inference context doesn't easily support that use case.

It would be good to get some concrete numbers about the actual impact of using
the AST `properties`.

[Jennifer Messerly, 2016-06-09 22:26:45]

On 2016/06/09 22:14:55, Leaf wrote:
> > Yeah I wondered that too :) ... was kind of thinking a warning because we
can
> > still compile/run code with it. And felt a bit more typical for compiler
flags
> > to just add warnings (vs errors). But yeah, definitely could make it an
error
> > too. Was a tossup in my brain. Either way, let me know. :)
> 
> I'd say make it an error.
> 
> > > 2) I think an alternative implementation approach would be to do this in
the
> > > analyzer proper (rather than in the strong mode checker). I think you
would
> > have
> > > to either push the analysis options into typeSystem so that the behavior
of
> > > isAssignable changed depending on whether or not this flag was set, or
else
> > > factor out a set of utility operations that checked the flag before
calling
> > > isAssignable.  A complication here is if you want to do different things
for
> > > assignments vs function arguments then you would need to get that
> information
> > to
> > > the right place.  Long term, I think we want to move more functionality
away
> > > from the strong mode checker, so it might be worth looking to see if this
is
> > > feasible.  But I'm fine with landing this as is if it seems better not to
> > start
> > > down the rabbit hole.
> > 
> > Yeah for sure, there's the full integration that we need to do to merge in
> > CodeChecker. That goes along with Brian's comment too (why do we have
another
> > thing that is almost but not quite an ErrorCode?). I retreated from all my
> > refactoring attempts after it seemed a bit more gnarly than I was ready to
> > tackle :). But I really want to get there. I see it as a bit of a multi-step
> > process:
> > 
> > - introduce some new AST properties for DDC to use to find coecions
> > - get DDC to stop using CoercionInfo and use the new properties
> > - introduce ErrorCodes and remove CoercionInfos
> > - migrate the downcast checking into resolver and/or error verifier
> > - migrate override checking somewhere (?)
> > - migrate dynamic call marking somewhere (? perhaps back into DDC)
> > - things are finally same again. celebrate \o/ !!!
> 
> I'm not sure I understand why all of these things would be on the
implementation
> path to doing this in the error-verifier/resolver.  My thinking (quite
possibly
> wrong) was that if you just changed the definition and/or uses of isAssignable
> to disable the additional backwards subtype checks when the
--no-implicit-casts
> flag was set, then all implicit cast locations would just show up as type
> errors.  The backend could keep generating coercion infos etc the same as it
> does now (maybe to be re-factored later).  Does this not work?  Or is it just
> that this would be detour from where you want to get to?

I'm super confused. Let me back up a bit and see if we can determine where I am
misunderstanding.

1. Right now we emit implicit cast messages here, in checker.dart/info.dart.
2. So I changed the code here.
3. You asked if I could make my fix "in analyzer proper".
4. I described the steps needed to get our implicit cast messages into "analyzer
proper".

I'm not sure where I went wrong but let me know. :|

In the meantime, I am hard at work integrating code into analyzer proper. I had
thought to land this CL to get the feature in for users, then continue
refactoring. But, I am getting closer to finishing the refactoring, so it may
become a moot point...

[Leaf, 2016-06-10 20:40:24]

lgtm with warning -> error.

[Jennifer Messerly, 2016-06-10 21:05:28]

On 2016/06/10 20:40:24, Leaf wrote:
> lgtm with warning -> error.

Done :)

[Jennifer Messerly, 2016-06-10 21:05:46]

Description was changed from

==========
Add analysis option that will be used to fix #26583

There's follow up work that is needed for CLI and analysis_options
==========

to

==========
Add analysis option that will be used to fix #26583

There's follow up work that is needed for CLI and analysis_options

R=brianwilkerson@google.com, leafp@google.com

Committed:
https://github.com/dart-lang/sdk/commit/67dd48060b127ebbaf67b091a2802bc728db3b50
==========

[Jennifer Messerly, 2016-06-10 21:05:48]

Committed patchset #2 (id:40001) manually as
67dd48060b127ebbaf67b091a2802bc728db3b50 (presubmit successful).

[Jennifer Messerly, 2016-06-10 21:13:10]

By the way, I've been busy in the meantime and will soon have a follow up to
address many of the high level ideas discussed here.

[Jennifer Messerly, 2016-06-10 22:58:22]

On 2016/06/10 21:13:10, John Messerly wrote:
> By the way, I've been busy in the meantime and will soon have a follow up to
> address many of the high level ideas discussed here.

And that CL is: https://codereview.chromium.org/2060013002/