Create a new AstFactory class to be used by analyzer and its clients.

Create a new AstFactory class to be used by analyzer and its clients.

Unlike the previous AstFactory (now called AstTestFactory), this class
is not just for testing; it is intended to be able to create all
analyzer AST objects used in production.  Also, its methods are
non-static.

For now, all users of the new AstFactory access it using the final
variable `astFactory` in standard_ast_factory.dart.  In future CLs I
intend to plumb a reference to the AstFactory through various analyzer
classes so that it can be used in a dependency injection style.

Also, the classes CommentType and UriValidationCodeImpl have been
moved out of the AST implementation file
(pkg/analyzer/lib/src/dart/ast/ast.dart) and to their own files so
that the AST interface will be able to continue to use them without
depending on the AST implementation.

In follow-up CLs I will change the analyzer and its clients to use the
AstFactory rather than the redirecting constructors in the AST
interface file, and then remove those redirecting constructors.

R=brianwilkerson@google.com

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

[Paul Berry, 2016-11-23 00:43:42]

paulberry@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2016-11-23 15:58:19]

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
File pkg/analyzer/lib/dart/ast/ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:12: * instances of the interfaces
that constitute the AST.
Clients should not be allowed to extend, implement or mix-in this class.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:16: * Initialize a newly created list
of adjacent strings. To be syntactically
Perhaps "Initialize" --> "Return"? I uniformly use "Initialize" for
constructors, which these are not.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:186: List<Token> tokens, CommentType
type, List<CommentReference> references);
This method should be removed in favor of the more specific methods:
blockComment, documentationComment, and endOfLineComment. Doing so should mean
that we do not need to make CommentType part of the public API.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:289: Comment
createBlockComment(List<Token> tokens);
This and the other "create" methods should have "create" dropped from the name
and the comment should be made consistent with the other methods "Return a newly
created ...".

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:300: Comment
createDocumentationCommentWithReferences(
We should remove this method and add an optional 'references' parameter to
createDocumentationComment.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
File pkg/analyzer/lib/dart/ast/standard_ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
pkg/analyzer/lib/dart/ast/standard_ast_factory.dart:11: final AstFactory
astFactory = new AstFactoryImpl();
Any chance that I could convince you to make this a factory method on
AstFactory, rather than a top-level variable in a separate library?

If not, can we at least move this into ast_factory.dart rather than having a
separate library for it?

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/ast/uri_validation_code.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/ast/uri_validation_code.dart:10: class
UriValidationCodeImpl implements UriValidationCode {
I would prefer to remove the method 'validate' from UriBasedDirective and to
remove UriValidationCode from the public API.

[Paul Berry, 2016-11-24 15:03:18]

PTAL

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
File pkg/analyzer/lib/dart/ast/ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:12: * instances of the interfaces
that constitute the AST.
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> Clients should not be allowed to extend, implement or mix-in this class.

Done.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:16: * Initialize a newly created list
of adjacent strings. To be syntactically
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> Perhaps "Initialize" --> "Return"? I uniformly use "Initialize" for
> constructors, which these are not.

Done.  Changed to "Returns" to be consistent with
https://www.dartlang.org/guides/language/effective-dart/documentation#prefer-...

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:186: List<Token> tokens, CommentType
type, List<CommentReference> references);
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> This method should be removed in favor of the more specific methods:
> blockComment, documentationComment, and endOfLineComment. Doing so should mean
> that we do not need to make CommentType part of the public API.

Done.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:289: Comment
createBlockComment(List<Token> tokens);
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> This and the other "create" methods should have "create" dropped from the name
> and the comment should be made consistent with the other methods "Return a
newly
> created ...".

Done.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast_factory.dart:300: Comment
createDocumentationCommentWithReferences(
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> We should remove this method and add an optional 'references' parameter to
> createDocumentationComment.

Done.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
File pkg/analyzer/lib/dart/ast/standard_ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
pkg/analyzer/lib/dart/ast/standard_ast_factory.dart:11: final AstFactory
astFactory = new AstFactoryImpl();
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> Any chance that I could convince you to make this a factory method on
> AstFactory, rather than a top-level variable in a separate library?
> 
> If not, can we at least move this into ast_factory.dart rather than having a
> separate library for it?

(Assuming you mean "move this into lib/dart/ast/ast_factory.dart", since we do
want this to remain in the public API).

One of the things I would like to do after importing some of this code into
front_end is to add a unit test that constrains the import relationships between
the files in front_end, to make sure that the import graph stays organized so
that a year from now we don't have everything in a single giant library cycle as
we currently do in analyzer.  In that unit test, I would like to make assertions
like "make sure the files in folder X only depend on the interface for ASTs, not
on their implementation".

The way the files are currently organized, it would be easy to check an
assertion like that using the import graph; we would simply check that the files
in folder X don't include lib/src/dart/ast/ast.dart (or whatever the path winds
up being inside front_end) in their transitive closure of imports.  But if we
made the change you suggest, that would introduce the import chain

lib/dart/ast/ast_factory.dart -> lib/src/dart/ast/ast_factory.dart ->
lib/src/dart/ast/ast.dart

and now we would no longer be able to tell by the import graph alone the
difference between a file that just depends on AstFactory and one that depends
on the AST implementation as well.

What's the end goal you are trying to achieve by moving this into
ast_factory.dart?  If there's a way we can achieve that end goal without losing
the ability to do the type of static analysis of the import graph I'm describing
here, I'd be happy to make the change.

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/ast/uri_validation_code.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/ast/uri_validation_code.dart:10: class
UriValidationCodeImpl implements UriValidationCode {
On 2016/11/23 15:58:18, Brian Wilkerson wrote:
> I would prefer to remove the method 'validate' from UriBasedDirective and to
> remove UriValidationCode from the public API. 

Done.

[Brian Wilkerson, 2016-11-28 16:27:39]

lgtm

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
File pkg/analyzer/lib/dart/ast/standard_ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
pkg/analyzer/lib/dart/ast/standard_ast_factory.dart:11: final AstFactory
astFactory = new AstFactoryImpl();
> What's the end goal you are trying to achieve by moving this into
ast_factory.dart?

I have a strong negative reaction to global state. How would you feel about
defining a top-level getter, rather than a top-level variable? (I have almost as
strong a negative reaction against top-level functions, but ...) While it's true
that there's no value to creating multiple factories, there's also no harm, and
it might discourage clients from directly referencing the global state rather
than allowing the factory to be injected into their classes.

Also, this seems like too small an amount of code to justify it being a separate
library, but I wasn't thinking about the dependency issue, so ...

[Paul Berry, 2016-11-28 18:02:30]

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
File pkg/analyzer/lib/dart/ast/standard_ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
pkg/analyzer/lib/dart/ast/standard_ast_factory.dart:11: final AstFactory
astFactory = new AstFactoryImpl();
Ok, since I got your LGTM, I'm going to land as is.  But I wanted to respond to
your points in an effort to try to find areas of agreement:

On 2016/11/28 16:27:38, Brian Wilkerson wrote:
> > What's the end goal you are trying to achieve by moving this into
> ast_factory.dart?
> 
> I have a strong negative reaction to global state.

Thanks for the explanation.  If it helps, I also have a strong negative reaction
to global state.  I believe it leads to bugs and discourages testability. 
However, while I understand that top level variables are a place in which global
state is often stored, I believe that it's a stretch to say that this is an
instance of global state; after all, the variable is final, and the class stored
in it does not contain any state in and of itself.  So I actually don't think of
this as a case of global state.  Maybe that just sounds like semantic mumbo
jumbo, but I want to reassure you that I don't like global state either, and I
don't believe that the introduction of this final top level variable is any
reason to worry that global state is beginning to creep into the code base. 
IMHO, it isn't, and I am as adamant as I've ever been that we should prevent it
from happening.

> How would you feel about
> defining a top-level getter, rather than a top-level variable? (I have almost
as
> strong a negative reaction against top-level functions, but ...) While it's
true
> that there's no value to creating multiple factories, there's also no harm,
and
> it might discourage clients from directly referencing the global state rather
> than allowing the factory to be injected into their classes.

If the goal is to influence client behavior into allowing the factory to be
injected, I think this change would actually not produce a very big effect,
because it would not make it any more difficult for the client to directly
reference the getter.  A more effective approach, IMHO, would be to get rid of
the final variable entirely and instead force clients to say "new
AstFactoryImpl()".  I considered that (and in fact I started out this change
that way), but after working with it for some time I realized that a lot of work
is still necessary before we can get to the point where the AST factory can be
effectively dependency injected: some of the code creating ASTs is in static
methods, or instance methods on inconvenient objects (such as parts of the
element model IIRC); that code is going to need a lot of reshaping before we can
really do the kind of dependency injection we want.  I really don't want to try
to use discouragement to influence clients into allowing dependency injection
when we are still several steps away from dependency injection really being a
realistic possibility.

If, on the other hand, the goal is to reassure ourselves that this top level
global doesn't introduce any global state, how would you feel about changing it
from a final variable to a const variable?  If we did that, then the VM and
analyzer would help us ensure that no actual global state existed.  (Of course
any of the methods on this object could themselves refer to global state; but
there's no way to avoid that, and indeed there's nothing stopping ordinary
constructors to refer to global state too).

> 
> Also, this seems like too small an amount of code to justify it being a
separate
> library, but I wasn't thinking about the dependency issue, so ...

Acknowledged.  This library is extremely small :)

[Brian Wilkerson, 2016-11-28 18:35:48]

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
File pkg/analyzer/lib/dart/ast/standard_ast_factory.dart (right):

https://codereview.chromium.org/2522143003/diff/1/pkg/analyzer/lib/dart/ast/s...
pkg/analyzer/lib/dart/ast/standard_ast_factory.dart:11: final AstFactory
astFactory = new AstFactoryImpl();
I would prefer that we didn't need this global member for now, and hope that we
can move to a world in which it isn't necessary at all, but I understand your
arguments and don't think you need to take any other actions at this time.

> I believe that it's a stretch to say that this is an instance of global state

That said, I disagree with the above claim :-). The global state it's
encapsulating is the set of implementation classes that will be created. Client
code can't change that. Of course, we don't *want* client code to provide
alternate implementations of AST nodes, so perhaps the global state is actually
a good thing in this case.

[Paul Berry, 2016-11-28 18:50:28]

Description was changed from

==========
Create a new AstFactory class to be used by analyzer and its clients.

Unlike the previous AstFactory (now called AstTestFactory), this class
is not just for testing; it is intended to be able to create all
analyzer AST objects used in production.  Also, its methods are
non-static.

For now, all users of the new AstFactory access it using the final
variable `astFactory` in standard_ast_factory.dart.  In future CLs I
intend to plumb a reference to the AstFactory through various analyzer
classes so that it can be used in a dependency injection style.

Also, the classes CommentType and UriValidationCodeImpl have been
moved out of the AST implementation file
(pkg/analyzer/lib/src/dart/ast/ast.dart) and to their own files so
that the AST interface will be able to continue to use them without
depending on the AST implementation.

In follow-up CLs I will change the analyzer and its clients to use the
AstFactory rather than the redirecting constructors in the AST
interface file, and then remove those redirecting constructors.
==========

to

==========
Create a new AstFactory class to be used by analyzer and its clients.

Unlike the previous AstFactory (now called AstTestFactory), this class
is not just for testing; it is intended to be able to create all
analyzer AST objects used in production.  Also, its methods are
non-static.

For now, all users of the new AstFactory access it using the final
variable `astFactory` in standard_ast_factory.dart.  In future CLs I
intend to plumb a reference to the AstFactory through various analyzer
classes so that it can be used in a dependency injection style.

Also, the classes CommentType and UriValidationCodeImpl have been
moved out of the AST implementation file
(pkg/analyzer/lib/src/dart/ast/ast.dart) and to their own files so
that the AST interface will be able to continue to use them without
depending on the AST implementation.

In follow-up CLs I will change the analyzer and its clients to use the
AstFactory rather than the redirecting constructors in the AST
interface file, and then remove those redirecting constructors.

R=brianwilkerson@google.com

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

[Paul Berry, 2016-11-28 18:50:29]

Committed patchset #2 (id:20001) manually as
f4baeb20d7b3a3a0819877af51acddc7bccd741d (presubmit successful).