Skip to content

Commit

Permalink
Use ParameterMember when substituting function types.
Browse files Browse the repository at this point in the history 
This ensures that the parameters from the function type still point to
the original function declaration (previously, they were synthetic
parameters).  This should lead to a better user experience in editor
integration, by ensuring that the editor can still find the parameter
declarations associated with parameters in generic function
invocations.

Fixes #48500.

Change-Id: I10a0dd54e7c70e1d0d05dced37565c35bcaabfd0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/243524
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
  • Loading branch information
@stereotype441
stereotype441 authored and Commit Bot committed May 12, 2022
1 parent e593b86 commit f5af2d9
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 34 deletions.
87 changes: 65 additions & 22 deletions pkg/analyzer/lib/src/dart/element/member.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class ConstructorMember extends ExecutableMember
/// Initialize a newly created element to represent a constructor, based on
/// the [declaration], and applied [substitution].
ConstructorMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
ConstructorElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -138,7 +138,7 @@ abstract class ExecutableMember extends Member implements ExecutableElement {
/// their bounds. The [substitution] includes replacing [declaration] type
/// parameters with the provided fresh [typeParameters].
ExecutableMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
ExecutableElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -178,6 +178,12 @@ abstract class ExecutableMember extends Member implements ExecutableElement {
@override
bool get isSynchronous => declaration.isSynchronous;

@override
LibraryElement get library => _declaration.library!;

@override
Source get librarySource => _declaration.librarySource!;

@override
List<ParameterElement> get parameters {
return declaration.parameters.map<ParameterElement>((p) {
Expand Down Expand Up @@ -220,7 +226,7 @@ abstract class ExecutableMember extends Member implements ExecutableElement {
ExecutableElement element,
MapSubstitution substitution,
) {
TypeProviderImpl typeProvider;
TypeProviderImpl? typeProvider;
var isLegacy = false;
var combined = substitution;
if (element is ExecutableMember) {
Expand Down Expand Up @@ -261,7 +267,7 @@ abstract class ExecutableMember extends Member implements ExecutableElement {
class FieldFormalParameterMember extends ParameterMember
implements FieldFormalParameterElement {
factory FieldFormalParameterMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
FieldFormalParameterElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand All @@ -280,7 +286,7 @@ class FieldFormalParameterMember extends ParameterMember
}

FieldFormalParameterMember._(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
FieldFormalParameterElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -320,7 +326,7 @@ class FieldMember extends VariableMember implements FieldElement {
/// Initialize a newly created element to represent a field, based on the
/// [declaration], with applied [substitution].
FieldMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
FieldElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -360,6 +366,9 @@ class FieldMember extends VariableMember implements FieldElement {
@override
bool get isExternal => declaration.isExternal;

@override
LibraryElement get library => _declaration.library!;

@override
String get name => declaration.name;

Expand Down Expand Up @@ -406,8 +415,8 @@ class FieldMember extends VariableMember implements FieldElement {
}

class FunctionMember extends ExecutableMember implements FunctionElement {
FunctionMember(
TypeProviderImpl typeProvider, FunctionElement declaration, bool isLegacy)
FunctionMember(TypeProviderImpl? typeProvider, FunctionElement declaration,
bool isLegacy)
: super(
typeProvider,
declaration,
Expand Down Expand Up @@ -441,7 +450,7 @@ class FunctionMember extends ExecutableMember implements FunctionElement {
/// parameters are known.
abstract class Member implements Element {
/// A type provider (might be legacy, might be null-safe).
final TypeProviderImpl _typeProvider;
final TypeProviderImpl? _typeProvider;

/// The element on which the parameterized element was created.
final Element _declaration;
Expand All @@ -459,6 +468,10 @@ abstract class Member implements Element {
if (_declaration is Member) {
throw StateError('Members must be created from a declarations.');
}
if (_typeProvider == null && isLegacy) {
throw StateError(
'A type provider must be supplied for legacy conversion');
}
}

@override
Expand Down Expand Up @@ -552,10 +565,10 @@ abstract class Member implements Element {
ElementKind get kind => _declaration.kind;

@override
LibraryElement get library => _declaration.library!;
LibraryElement? get library => _declaration.library;

@override
Source get librarySource => _declaration.librarySource!;
Source? get librarySource => _declaration.librarySource;

@override
ElementLocation get location => _declaration.location!;
Expand Down Expand Up @@ -642,7 +655,7 @@ abstract class Member implements Element {
/// Otherwise, return the type unchanged.
DartType _toLegacyType(DartType type) {
if (isLegacy) {
return NullabilityEliminator.perform(_typeProvider, type);
return NullabilityEliminator.perform(_typeProvider!, type);
} else {
return type;
}
Expand Down Expand Up @@ -720,7 +733,7 @@ abstract class Member implements Element {
/// type parameters are known.
class MethodMember extends ExecutableMember implements MethodElement {
factory MethodMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
MethodElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand All @@ -739,7 +752,7 @@ class MethodMember extends ExecutableMember implements MethodElement {
}

MethodMember._(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
MethodElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -799,7 +812,7 @@ class ParameterMember extends VariableMember
final List<TypeParameterElement> typeParameters;

factory ParameterMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
ParameterElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand All @@ -820,7 +833,7 @@ class ParameterMember extends VariableMember
/// Initialize a newly created element to represent a parameter, based on the
/// [declaration], with applied [substitution].
ParameterMember._(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
ParameterElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -887,14 +900,41 @@ class ParameterMember extends VariableMember
super.visitChildren(visitor);
safelyVisitChildren(parameters, visitor);
}

static ParameterElement from(
ParameterElement element, MapSubstitution substitution) {
TypeProviderImpl? typeProvider;
var isLegacy = false;
var combined = substitution;
if (element is ParameterMember) {
var member = element;
element = member.declaration;
typeProvider = member._typeProvider;

isLegacy = member.isLegacy;

var map = <TypeParameterElement, DartType>{};
for (var entry in member._substitution.map.entries) {
map[entry.key] = substitution.substituteType(entry.value);
}
map.addAll(substitution.map);
combined = Substitution.fromMap(map);
}

if (!isLegacy && combined.map.isEmpty) {
return element;
}

return ParameterMember(typeProvider, element, combined, isLegacy);
}
}

/// A property accessor element defined in a parameterized type where the values
/// of the type parameters are known.
class PropertyAccessorMember extends ExecutableMember
implements PropertyAccessorElement {
factory PropertyAccessorMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
PropertyAccessorElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand All @@ -913,7 +953,7 @@ class PropertyAccessorMember extends ExecutableMember
}

PropertyAccessorMember._(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
PropertyAccessorElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -1005,7 +1045,7 @@ class PropertyAccessorMember extends ExecutableMember
class SuperFormalParameterMember extends ParameterMember
implements SuperFormalParameterElement {
factory SuperFormalParameterMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
SuperFormalParameterElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand All @@ -1024,7 +1064,7 @@ class SuperFormalParameterMember extends ParameterMember
}

SuperFormalParameterMember._(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
SuperFormalParameterElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -1063,7 +1103,7 @@ class SuperFormalParameterMember extends ParameterMember
class TopLevelVariableMember extends VariableMember
implements TopLevelVariableElement {
TopLevelVariableMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
VariableElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down Expand Up @@ -1092,6 +1132,9 @@ class TopLevelVariableMember extends VariableMember
@override
bool get isExternal => declaration.isExternal;

@override
LibraryElement get library => _declaration.library!;

@override
String get name => declaration.name;

Expand Down Expand Up @@ -1119,7 +1162,7 @@ abstract class VariableMember extends Member implements VariableElement {
/// Initialize a newly created element to represent a variable, based on the
/// [declaration], with applied [substitution].
VariableMember(
TypeProviderImpl typeProvider,
TypeProviderImpl? typeProvider,
VariableElement declaration,
MapSubstitution substitution,
bool isLegacy,
Expand Down
6 changes: 2 additions & 4 deletions pkg/analyzer/lib/src/dart/element/type.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import 'package:analyzer/dart/element/type_visitor.dart';
import 'package:analyzer/src/dart/analysis/session.dart';
import 'package:analyzer/src/dart/element/display_string_builder.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/extensions.dart';
import 'package:analyzer/src/dart/element/inheritance_manager3.dart';
import 'package:analyzer/src/dart/element/member.dart';
import 'package:analyzer/src/dart/element/type_algebra.dart';
Expand Down Expand Up @@ -224,9 +223,8 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
return FunctionTypeImpl(
returnType: substitution.substituteType(returnType),
typeFormals: const [],
parameters: parameters
.map((p) => p.copyWith(type: substitution.substituteType(p.type)))
.toList(),
parameters:
parameters.map((p) => ParameterMember.from(p, substitution)).toList(),
nullabilitySuffix: nullabilitySuffix,
);
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/analyzer/lib/src/test_utilities/mock_sdk.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,9 @@ abstract class Set<E> implements Iterable<E> {
void addAll(Iterable<E> elements);
bool remove(Object? value);
E? lookup(Object? object);
static Set<T> castFrom<S, T>(Set<S> source, {Set<R> Function<R>()? newSet}) =>
throw '';
}
class StackTrace {}
Expand Down
45 changes: 37 additions & 8 deletions pkg/analyzer/test/generated/non_error_resolver_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1458,10 +1458,8 @@ test(C c) => c.method<bool>(arg: true);
''');
var x = findNode.namedExpression('arg: true');
var y = x.staticParameterElement!;
// Note: the staticParameterElement is synthetic; see
// https://github.com/dart-lang/sdk/issues/48500
expect(y, isNot(TypeMatcher<ParameterMember>()));
expect(y.enclosingElement, isNull);
expect(y.enclosingElement, isNotNull);
expect(y.declaration, findElement.parameter('arg'));
}

test_generic_staticParameterElement_methodCall_implicitTypeArg() async {
Expand All @@ -1473,10 +1471,8 @@ bool test(C c) => c.method<bool>(arg: true);
''');
var x = findNode.namedExpression('arg: true');
var y = x.staticParameterElement!;
// Note: the staticParameterElement is synthetic; see
// https://github.com/dart-lang/sdk/issues/48500
expect(y, isNot(TypeMatcher<ParameterMember>()));
expect(y.enclosingElement, isNull);
expect(y.enclosingElement, isNotNull);
expect(y.declaration, findElement.parameter('arg'));
}

test_genericTypeAlias_castsAndTypeChecks_hasTypeParameters() async {
Expand Down Expand Up @@ -2258,6 +2254,21 @@ int f(v) {
''');
}

test_librarySource_of_type_substituted_synthetic_parameter() async {
await assertErrorsInCode('''
Map<int, T> f<T>(T t) => throw '';
Map<double, T> g<T>(T t) => throw '';
h(bool b) {
Map<num, String> m = (b ? f : g)('x');
}
''', [
error(HintCode.UNUSED_LOCAL_VARIABLE, 104, 1),
]);
var parameter = findNode.stringLiteral("'x'").staticParameterElement;
expect(parameter!.library, isNull);
expect(parameter.librarySource, isNull);
}

test_loadLibraryDefined() async {
newFile('$testPackageLibPath/lib.dart', r'''
library lib;
Expand Down Expand Up @@ -3310,6 +3321,24 @@ core.dynamic dynamicVariable;
@reflectiveTest
class NonErrorResolverWithoutNullSafetyTest extends PubPackageResolutionTest
with WithoutNullSafetyMixin, NonErrorResolverTestCases {
test_castFrom() async {
// This test exercises a corner case of legacy erasure: due to the type
// substitution in the `newSet` parameter of `Set.castFrom`, we wind up with
// a synthetic `ParameterMember` that belongs to no library. We need to
// make sure this doesn't lead to a crash.
await assertErrorsInCode('''
class C {}
void testNewSet(Set<C> setEls) {
var customNewSet;
Set.castFrom<C, Object>(setEls,
newSet: <T>() => customNewSet = new Set<T>());
}
''', [
error(HintCode.UNUSED_LOCAL_VARIABLE, 51, 12),
]);
}

test_conflictingStaticGetterAndInstanceSetter_thisClass() async {
await assertNoErrorsInCode(r'''
class A {
Expand Down
15 changes: 15 additions & 0 deletions pkg/analyzer/test/src/dart/resolution/constructor_reference_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,21 @@ ConstructorReference
''');
}

test_alias_generic_with_inferred_type_parameter() async {
await assertErrorsInCode('''
class C<T> {
final T x;
C(this.x);
}
typedef Direct<T> = C<T>;
void main() {
var x = const <C<int> Function(int)>[Direct.new];
}
''', [
error(HintCode.UNUSED_LOCAL_VARIABLE, 87, 1),
]);
}

test_alias_genericWithBound_unnamed() async {
await assertNoErrorsInCode('''
class A<T> {
Expand Down

0 comments on commit f5af2d9

Please sign in to comment.