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

pkg/analyzer/lib/dart/ast/standard_ast_factory.dart

+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:analyzer/dart/ast/ast_factory.dart';
+import 'package:analyzer/src/dart/ast/ast_factory.dart';
+
+/**
+ * Gets an instance of [AstFactory] based on the standard AST implementation.
+ */
+final AstFactory astFactory = new AstFactoryImpl();

[Brian Wilkerson, 2016/11/23 15:58:18]

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?

[Paul Berry, 2016/11/24 15:03:17]

(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.

[Brian Wilkerson, 2016/11/28 16:27:38]

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]

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:
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.
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).
Acknowledged.  This library is extremely small :)

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

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.
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.