In the two LibraryAnalyzer classes, the list of visitors is always
empty, so there are never any "visitor based lints" to run. (I
suspect that all the visitor based lints were long ago converted to
use the node registry, leaving this code dead).
Removing the last remnants of the visitor based lint logic allows us
to remove the ExceptionHandlingDelegatingAstVisitor class, which in
turn allows us to move its logException method into a simpler class.
In a follow-up CL, I will expand on this class to allow its behavior
to be customized.
Change-Id: I1261a94572ea65eb220a58fb45f6d887e6497fe9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208140
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
We need to be careful that when analyzing a declaration that
introduces a new scope, any annotations on that declaration are
considered to be outside of the newly introduced scope. Otherwise, we
might incorrectly analyze code like this:
const A = ...;
@A // Resolves to the const above, not the type parameter below
class C<A> {}
Previously, the way we handled most cases like this was as follows
(considering visitClassDeclaration as an example)
1. ResolverVisitor.visitClassDeclaration visited annotations first,
before any new scopes had been created; this ensured that the
annotations were resolved in the correct scope.
2. ResolverVisitor.visitClassDeclaration set the
`_enclosingClassDeclaration` field to point to the ClassDeclaration
node.
3. ResolverVisitor.visitClassDeclaration called
ScopedVisitor.visitClassDeclaration, which created two new scopes
for the class (one containing just type parameters, and one
containing both type parameters and members); then it visited all
the child nodes of the ClassDeclaration (including annotations)
with one of those scopes active. (This was incorrect, since
neither of the two class scopes should apply to the annotations!)
4. When ResolverVisitor.visitAnnotation was reached, it noticed that
the parent of the annotation matched `_enclosingClassDeclaration`,
so it did an early return; this avoided overwriting the correct
resolution produced in step 1.
This approach had some disadvantages:
- It meant that the scopes for annotations were only correct when
using the ResolverVisitor. The other class derived from
ScopedVisitor (VariableResolverVisitor) got the scopes for
annotations wrong. This was benign, but it was getting in the way
of my efforts to unify resolution logic between the analyzer and
CFE.
- It was only properly implemented for ClassDeclaration,
FunctionTypeAlias, and MixinDeclaration; annotations were
incorrectly scoped for ClassTypeAlias, EnumDeclaration,
ExtensionDeclaration, FunctionDeclaration,
FunctionTypedFormalParameter, GenericTypeAlias, and
MethodDeclaration.
This CL changes the approach so that ScopedVisitor always visits
annotations before creating scopes; this ensures that annotation
scoping is correct for both ResolverVisitor and
VariableResolverVisitor, and avoids the need for an early return from
the visitAnnotation method.
With this change, a few of the ScopedVisitor methods no longer wind up
making a super call into UnifyingAstVisitor (visitClassTypeAlias,
visitFunctionDeclarationInScope, visitFunctionTypeAliasInScope,
visitFunctionTypedFormalParameter, visitGenericTypeAlias, and
visitMethodDeclarationInScope); accordingly, I've updated the
ResolverVisitor to ensure that the elementResolver will still be
invoked for all these AST structures. (Previously this happened
automatically because the UnifyingAstVisitor would call visitNode).
I've included several tests to ensure that we don't regress the
analyzer functionality that is fixed by this change.
Change-Id: I6c79ff8a12301e0300f0679acb489cce86f25955
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/200881
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
So far, comment references with a prefix identifier were only resolved
against a named library import or a class.
Dartdoc also supports referencing extension members with that syntax, so
the analyzer should be able to resolve those as well.
Change-Id: I6e57e4e09b7ebcbc543b198950f9ee5919dbe104
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204040
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
The new "constructor tearoffs" feature includes the ability to use the
`new` keyword in a place where an identifier is expected, and that
identifier could denote a constructor name. We handle this by
replacing the `new` keyword token with an identifier token whose
identifier string is `new` (this should ease the burden on the
implementations, since they are already set up to handle constructor
names that are identifiers).
Since all such situations follow a `.`, and `new` was never previously
allowed after a `.`, the parser treats `new` as an identifier in any
situation where it could possibly refer to a constructor, regardless
of whether the "constructor tearoffs" feature is enabled. (This
should allow for easier error recovery in the situation where the user
tries to use the feature with a language feature that does not support
it). It is up to the client to report an error if the feature is
disabled.
In this CL, I've implemented logic for the analyzer to choose whether
to report an error based on whether the feature is enabled. I've
implemented logic for the CFE to report the error unconditionally.
Bug: https://github.com/dart-lang/sdk/issues/46020, https://github.com/dart-lang/sdk/issues/46044
Change-Id: I36a496688400d2d9f699dd42be4d0ba620cda244
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201961
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Now that we have an AST and visitor support for the FunctionReference
structure (which represents `Expression<TypeArguments>` for various
kinds of expressions), we no longer need to error recover this as a
FunctionExpressionInvocation with synthetic arguments. The resolver
still doesn't resolve the syntax properly, but that's ok because it's
not permitted in valid code (for now we just treat it as having type
`dynamic`).
Fixes#46150.
Change-Id: I357175cc16bcf2f9027be2e1da66bb6ca70a9400
Bug: https://github.com/dart-lang/sdk/issues/46020
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199682
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Previously, if a keyword was used in place of a constructor name in a
constructor initializer list, it caused an analyzer crash, because as
part of error recovery, the AST builder would try to create a
SuperConstructorInvocation or a RedirectingConstructorInvocation using
a null argument list. This CL fixes the problem by creating a
synthetic argument list. It re-uses logic that previously existed for
creating a synthetic argument list.
To avoid a crash happening later in analysis, it was also necessary to
modify the AST cloner so that when it encounters the `(` and `)`
tokens in the synthetic argument list, it is able to clone them even
though they are not in the token stream. (Note that this is not an
ideal solution; I would have rather inserted the `(` and `)` into the
token stream, but with the current parser architecture there's no good
way to do this without the parser then trying to interpret those
tokens).
Change-Id: Ibbdcdd956d80c16d427ba1ba7a9cd7ce374e941b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201282
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
There are several places in linter where we cast unconditionally.
Potentially we could also have `firstExpression` because there is
always at least one.
Change-Id: I9c3e754de6c8872941c1f1a47e36a6cbd3714b4a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201362
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This CL adds parser support for use of `<typeArguments>` as a
selector. This allows expressions like `List<int>` (type literal with
type arguments), `f<int>` (function tear-off with type arguments),
`C.m<int>` (static method tear-off with type arguments), `EXPR.m<int>`
(instance method tear-off with type arguments), and `EXPR<int>`
(tear-off of `.call` method with type arguments).
I will add parser support for `.new` as a constructor name in a
follow-up CL.
Change-Id: I157e732276421e8c3fd20c38c67ae9643993bd85
Bug: https://github.com/dart-lang/sdk/issues/46020, https://github.com/dart-lang/sdk/issues/46044.
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197102
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
This paves the way for allowing analyzer and analysis server clients
to format diagnostic message URLs in a special way (e.g. to make them
clickable).
Note that DiagnosticMessage is part of the public API of the analyzer,
so I've retained the old behavior in a deprecated fashion to avoid
breaking clients that don't yet handle diagnostic messages containing
URLs.
See
https://dart-review.googlesource.com/c/sdk/+/193749/comment/86d1ce4b_77a60b1e/
for additional discussion.
Change-Id: Iae9d43a2be7dbc67cb7cb82afe0a7824043d6113
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196101
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Konstantin Shcheglov