Skip to content

Commit

Permalink
Add type tracking to type inference logging.
Browse files Browse the repository at this point in the history 
This commit updates the type inference logging logic introduced in
https://dart-review.googlesource.com/c/sdk/+/369788 so that it
additionally tracks the context and static type for each inferred
expression.

Tracking the context for each expression is easy, since it is an input
to the type inference algorithm, passed in to each `visit` method of
`ResolverVisitor` through the named parameter `contextType`. However,
there were a few expression types for which the context type *wasn't*
passed in, because it wasn't used (for example
`ResolverVisitor.visitBooleanLiteral`, since boolean literals always
have the same meaning regardless of their context). `contextType`
parameters have been added to these `visit` methods for consistency
with the other expression visit methods, so that the type inference
log shows the context for all expressions, whether it makes a
difference to inference or not.

Tracking the static type for each expression is a little trickier,
since it's not an explicit output of the type inference algorithm, but
rather the static type of each expression is set as a side effect of
the type inference mechanism. To make things more tractable, the
`ExpressionImpl.staticType` field is made private, and instead of
setting it directly, the resolver must set it by either calling
`recordStaticType` or `setPseudoExpressionStaticType`. The former is
used when resolving a real expression; the latter is used for
situations where the analyzer assigns a static type to an AST node
even though that AST node isn't really serving as an expression
according to the official language specification. (For example, when
analyzing the method invocation `x.foo()`, the analyzer stores a
static type on the SimpleIdentifier `foo`, even though according to
the language spec, `foo` in this context actually isn't an expression
in its own right).

Splitting the code paths that set static types into `recordStaticType`
and `setPseudoExpressionStaticType` allows for the type inference
logging mechanism to check some useful invariants: it verifies that
every expression that the resolver visits is either assigned a static
type exactly once through a call to `recordStaticType`, or it's
determined to not be a true expression (and hence not assigned a
static type at all); I believe the latter happens mostly when
analyzing erroneous code, or when the resolver visitor is called upon
to assign a type to an identifier that's in a declaration context.

Change-Id: Icdf023d03fba3c87dbec3a72d00d0e9c7d1da5fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370322
Reviewed-by: Chloe Stefantsova <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
  • Loading branch information
@stereotype441
stereotype441 authored and Commit Queue committed Jun 10, 2024
1 parent 8e219fc commit 172bbe0
Show file tree
Hide file tree
Showing 26 changed files with 423 additions and 334 deletions.
83 changes: 69 additions & 14 deletions pkg/_fe_analyzer_shared/lib/src/type_inference/shared_inference_log.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// 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 '../types/shared_type.dart';

/// Maximum length for strings returned by [describe].
const int _descriptionLengthThreshold = 80;

Expand Down Expand Up @@ -56,22 +58,38 @@ class Event {
Event({required this.message});
}

/// Specialization of [State] used when type inferring an expression.
class ExpressionState extends State {
/// Whether [SharedInferenceLogWriterImpl.recordStaticType] or
/// [SharedInferenceLogWriterImpl.recordExpressionWithNoType] has been called
/// for the expression represented by this [State] object.
///
/// The inference log infrastructure uses this boolean to verify that each
/// expression that undergoes type inference either receives a static type, or
/// is determined by analysis to not need a static type.
bool typeRecorded = false;

ExpressionState(
{required super.writer, required super.message, required super.nodeSet})
: super(kind: StateKind.expression);
}

/// Public API to the interface log writer.
///
/// This class defines methods that the analyzer or CFE can use to instrument
/// their type inference logic. The implementations are found in
/// [SharedInferenceLogWriterImpl].
abstract interface class SharedInferenceLogWriter {
abstract interface class SharedInferenceLogWriter<Type extends SharedType> {
/// Verifies that every call to an `enter...` method has been matched by a
/// corresponding call to an `exit...` method.
void assertIdle();

/// Called when type inference begins inferring an expression.
void enterExpression(Object node);
void enterExpression(Object node, Type contextType);

/// Called when type inference begins inferring an AST node associated with
/// extension override syntax.
void enterExtensionOverride(Object node);
void enterExtensionOverride(Object node, Type contextType);

/// Called when type inference has discovered that a construct that uses
/// method invocation syntax (e.g. `x.f()`) is actually an invocation of a
Expand Down Expand Up @@ -114,6 +132,14 @@ abstract interface class SharedInferenceLogWriter {
/// double check that it's only recorded once in the log.
void recordExpressionRewrite(
{Object? oldExpression, required Object newExpression});

/// Called when type inference is inferring an expression, and discovers that
/// the expression should not have any type.
void recordExpressionWithNoType(Object expression);

/// Called when type inference is inferring an expression, and assigns the
/// expression a static type.
void recordStaticType(Object expression, Type type);
}

/// Implementation of the interface log writer.
Expand All @@ -125,8 +151,8 @@ abstract interface class SharedInferenceLogWriter {
/// from classes derived from [SharedInferenceLogWriterImpl], but these methods
/// are not exposed in [SharedInferenceLogWriter] so that they won't be called
/// accidentally on their own.
abstract class SharedInferenceLogWriterImpl
implements SharedInferenceLogWriter {
abstract class SharedInferenceLogWriterImpl<Type extends SharedType>
implements SharedInferenceLogWriter<Type> {
/// A stack of [State] objects representing the calls that have been made to
/// `enter...` methods without any matched `exit...` method.
///
Expand Down Expand Up @@ -237,27 +263,26 @@ abstract class SharedInferenceLogWriterImpl
}

@override
void enterExpression(Object node) {
pushState(new State(
kind: StateKind.expression,
void enterExpression(Object node, Type contextType) {
pushState(new ExpressionState(
writer: this,
message: 'INFER EXPRESSION ${describe(node)}',
message: 'INFER EXPRESSION ${describe(node)} IN CONTEXT $contextType',
nodeSet: [node]));
}

@override
void enterExtensionOverride(Object node) {
void enterExtensionOverride(Object node, Type contextType) {
pushState(new State(
kind: StateKind.extensionOverride,
writer: this,
message: 'INFER EXTENSION OVERRIDE ${describe(node)}',
message: 'INFER EXTENSION OVERRIDE ${describe(node)} IN CONTEXT '
'$contextType',
nodeSet: [node]));
}

@override
void enterFunctionExpressionInvocationTarget(Object node) {
pushState(new State(
kind: StateKind.expression,
pushState(new ExpressionState(
writer: this,
message: 'REINTERPRET METHOD NAME ${describe(node)} AS AN EXPRESSION',
nodeSet: [node]));
Expand All @@ -280,8 +305,14 @@ abstract class SharedInferenceLogWriterImpl
namedArguments: {if (reanalyze) 'reanalyze': reanalyze},
expectedNode: node,
expectedKind: StateKind.expression);
bool typeRecorded = (state as ExpressionState).typeRecorded;
if (reanalyze) {
if (typeRecorded) {
fail('Tried to reanalyze after already recording a static type');
}
addEvent(new Event(message: 'WILL REANALYZE AS OTHER EXPRESSION'));
} else if (!typeRecorded) {
fail('Failed to record a type for $state');
}
popState();
}
Expand Down Expand Up @@ -349,14 +380,38 @@ abstract class SharedInferenceLogWriterImpl
addEvent(new Event(
message: 'REWRITE ${describe(oldExpression)} TO '
'${describe(newExpression)}'));
state.nodeSet.add(newExpression);
(state as ExpressionState).nodeSet.add(newExpression);
if (oldExpression != null) {
if (_rewrittenExpressions[oldExpression] ?? false) {
fail('Expression already rewritten: ${describe(oldExpression)}');
}
_rewrittenExpressions[oldExpression] = true;
}
}

@override
void recordExpressionWithNoType(Object expression) {
checkCall(
method: 'recordExpressionWithNoType',
arguments: [expression],
expectedNode: expression,
expectedKind: StateKind.expression);
addEvent(
new Event(message: 'EXPRESSION ${describe(expression)} HAS NO TYPE'));
(state as ExpressionState).typeRecorded = true;
}

@override
void recordStaticType(Object expression, Type type) {
checkCall(
method: 'recordStaticType',
arguments: [expression, type],
expectedNode: expression,
expectedKind: StateKind.expression);
addEvent(
new Event(message: 'STATIC TYPE OF ${describe(expression)} IS $type'));
(state as ExpressionState).typeRecorded = true;
}
}

/// Specialization of [Event] representing an event that might be associated
Expand Down
66 changes: 46 additions & 20 deletions pkg/analyzer/lib/src/dart/ast/ast.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ final class AdjacentStringsImpl extends StringLiteralImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitAdjacentStrings(this);
resolver.visitAdjacentStrings(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -1715,7 +1715,7 @@ final class AugmentedExpressionImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitAugmentedExpression(this);
resolver.visitAugmentedExpression(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -2135,7 +2135,7 @@ final class BooleanLiteralImpl extends LiteralImpl implements BooleanLiteral {

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitBooleanLiteral(this);
resolver.visitBooleanLiteral(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -5346,7 +5346,7 @@ final class DoubleLiteralImpl extends LiteralImpl implements DoubleLiteral {

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitDoubleLiteral(this);
resolver.visitDoubleLiteral(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -5992,8 +5992,7 @@ final class ExpressionFunctionBodyImpl extends FunctionBodyImpl

sealed class ExpressionImpl extends AstNodeImpl
implements CollectionElementImpl, Expression {
@override
DartType? staticType;
DartType? _staticType;

@override
bool get inConstantContext {
Expand Down Expand Up @@ -6076,9 +6075,24 @@ sealed class ExpressionImpl extends AstNodeImpl
return null;
}

@override
DartType? get staticType => _staticType;

@override
ExpressionImpl get unParenthesized => this;

/// Record that the static type of the given node is the given type.
///
/// @param expression the node whose type is to be recorded
/// @param type the static type of the node
void recordStaticType(DartType type, {required ResolverVisitor resolver}) {
_staticType = type;
if (type.isBottom) {
resolver.flowAnalysis.flow?.handleExit();
}
inferenceLogWriter?.recordStaticType(this, type);
}

@override
void resolveElement(
ResolverVisitor resolver, CollectionLiteralContext? context) {
Expand All @@ -6093,6 +6107,17 @@ sealed class ExpressionImpl extends AstNodeImpl
/// call [ResolverVisitor.dispatchExpression], which has some special logic
/// for handling dynamic contexts.
void resolveExpression(ResolverVisitor resolver, DartType contextType);

/// Records that the static type of `this` is [type], without triggering any
/// [ResolverVisitor] behaviors.
///
/// This is used when the expression AST node occurs in a place where it is
/// not technically a true expression, but the analyzer chooses to assign it a
/// static type anyway (e.g. the [SimpleIdentifier] representing the method
/// name in a method invocation).
void setPseudoExpressionStaticType(DartType? type) {
_staticType = type;
}
}

/// An expression used as a statement.
Expand Down Expand Up @@ -6527,7 +6552,7 @@ final class ExtensionOverrideImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitExtensionOverride(this);
resolver.visitExtensionOverride(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -8443,7 +8468,7 @@ final class FunctionReferenceImpl extends CommentReferableExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitFunctionReference(this);
resolver.visitFunctionReference(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -10527,7 +10552,7 @@ final class IsExpressionImpl extends ExpressionImpl implements IsExpression {

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitIsExpression(this);
resolver.visitIsExpression(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -12876,7 +12901,7 @@ final class NullLiteralImpl extends LiteralImpl implements NullLiteral {

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitNullLiteral(this);
resolver.visitNullLiteral(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -13415,7 +13440,7 @@ final class PatternAssignmentImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitPatternAssignment(this);
resolver.visitPatternAssignment(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -14960,7 +14985,7 @@ final class RethrowExpressionImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitRethrowExpression(this);
resolver.visitRethrowExpression(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -15707,7 +15732,7 @@ final class SimpleStringLiteralImpl extends SingleStringLiteralImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitSimpleStringLiteral(this);
resolver.visitSimpleStringLiteral(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -15951,7 +15976,7 @@ final class StringInterpolationImpl extends SingleStringLiteralImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitStringInterpolation(this);
resolver.visitStringInterpolation(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -16215,7 +16240,7 @@ final class SuperExpressionImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitSuperExpression(this);
resolver.visitSuperExpression(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -16650,11 +16675,12 @@ final class SwitchExpressionImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
inferenceLogWriter?.enterExpression(this);
inferenceLogWriter?.enterExpression(this, contextType);
var previousExhaustiveness = resolver.legacySwitchExhaustiveness;
staticType = resolver
var staticType = resolver
.analyzeSwitchExpression(this, expression, cases.length, contextType)
.type;
recordStaticType(staticType, resolver: resolver);
resolver.popRewrite();
resolver.legacySwitchExhaustiveness = previousExhaustiveness;
inferenceLogWriter?.exitExpression(this);
Expand Down Expand Up @@ -16971,7 +16997,7 @@ final class SymbolLiteralImpl extends LiteralImpl implements SymbolLiteral {

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitSymbolLiteral(this);
resolver.visitSymbolLiteral(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -17097,7 +17123,7 @@ final class ThrowExpressionImpl extends ExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitThrowExpression(this);
resolver.visitThrowExpression(this, contextType: contextType);
}

@override
Expand Down Expand Up @@ -17552,7 +17578,7 @@ final class TypeLiteralImpl extends CommentReferableExpressionImpl

@override
void resolveExpression(ResolverVisitor resolver, DartType contextType) {
resolver.visitTypeLiteral(this);
resolver.visitTypeLiteral(this, contextType: contextType);
}

@override
Expand Down
4 changes: 2 additions & 2 deletions pkg/analyzer/lib/src/dart/constant/evaluation.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2542,7 +2542,7 @@ class _InstanceCreationEvaluator {
StringToken(TokenType.STRING, parameter.name, -1),
)
..staticElement = parameter
..staticType = parameter.type;
..setPseudoExpressionStaticType(parameter.type);
if (parameter.isPositional) {
superArguments.insert(positionalIndex++, value);
} else {
Expand All @@ -2555,7 +2555,7 @@ class _InstanceCreationEvaluator {
colon: StringToken(TokenType.COLON, ':', -1),
),
expression: value,
)..staticType = value.typeOrThrow,
)..setPseudoExpressionStaticType(value.typeOrThrow),
);
}
}
Expand Down
Loading

0 comments on commit 172bbe0

Please sign in to comment.