Skip to content

Commit 8d0dadd

Browse files
hamishknighthamishknight
committed
[Parse][Sema] Avoid overlapping fix-it removal ranges for redundant ': Any' constraints and conformances
This is in order to avoid emitting removal fix-its that would require their source range to be re-calculated after the application of another fix-it. FIX-ME(SR-8102): Improve the fix-it engine to handle such 'dependant' removal fix-its.
1 parent 099430d commit 8d0dadd

File tree

8 files changed

+181
-50
lines changed

8 files changed

+181
-50
lines changed

‎include/swift/AST/Decl.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,13 +1183,13 @@ class RequirementRepr {
11831183
return SeparatorLoc;
11841184
}
11851185

1186-
/// Retrieve the source range suitable for removing the requirement. Unlike
1187-
/// \c getSourceRange(), this range includes everything that needs to be
1188-
/// removed to keep the program syntactically valid (e.g. the 'where' keyword
1189-
/// and commas).
1186+
/// Retrieve the source range suitable for removing the requirement, if any.
1187+
/// Unlike \c getSourceRange(), this range includes everything that needs to
1188+
/// be removed to keep the program syntactically valid (e.g. the 'where'
1189+
/// keyword and commas).
11901190
SourceRange getRemovalRange() const { return RemovalRange; }
11911191

1192-
/// Set the source range suitable for removing the requirement. Unlike
1192+
/// Set the source range suitable for removing the requirement, if any. Unlike
11931193
/// \c getSourceRange(), this range includes everything that needs to be
11941194
/// removed to keep the program syntactically valid (e.g. the 'where' keyword
11951195
/// and commas).

‎lib/Parse/ParseGeneric.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -375,15 +375,19 @@ ParserStatus Parser::parseGenericWhereClause(
375375
// requirement.
376376
removalRange.Start = WhereLoc;
377377
}
378-
if (!HasNextReq && LastCommaLoc.isValid()) {
379-
// Include the previous comma in the removal range of the last
380-
// requirement.
381-
removalRange.Start = LastCommaLoc;
382-
}
383378
if (HasNextReq) {
384379
// Include the next comma in the removal range of a middle requirement.
385380
removalRange.End = NextCommaLoc;
386381
}
382+
if (!HasNextReq && LastCommaLoc.isValid()) {
383+
// HACK: Don't provide removal ranges for last-but-not-only requirements.
384+
// This is in order to avoid emitting removal fix-its that would require
385+
// their source range to be re-calculated after the application of another
386+
// fix-it.
387+
// FIX-ME(SR-8102): Improve the fix-it engine to handle such 'dependant'
388+
// removal fix-its.
389+
removalRange = SourceRange();
390+
}
387391
req.setRemovalRange(removalRange);
388392

389393
// If there's a comma, keep parsing the list.

‎lib/Sema/TypeCheckDecl.cpp

Lines changed: 50 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,29 @@ void TypeChecker::checkInheritanceClause(Decl *decl,
363363
return SourceRange(afterPriorLoc, afterMyEndLoc);
364364
};
365365

366+
bool didFixItRemoveFirst = false;
367+
368+
/// Attempt to emit a removal fix-it for the ith entry in the inheritance
369+
/// clause. Returns \c true if a removal fix-it was emitted, \c false
370+
/// otherwise.
371+
auto tryAddRemovalFixIt = [&](InFlightDiagnostic &diag, unsigned i) -> bool {
372+
// HACK: Don't provide a removal fix-it for the second entry if we provided
373+
// a removal fix-it for the first entry. This is in order to avoid emitting
374+
// removal fix-its that would require their source range to be re-calculated
375+
// after the application of another fix-it (which currently causes issues
376+
// with the the bulk application of fix-its).
377+
// FIX-ME(SR-8102): Improve the fix-it engine to handle such 'dependant'
378+
// removal fix-its.
379+
if (i == 0)
380+
didFixItRemoveFirst = true;
381+
else if (i == 1 && didFixItRemoveFirst)
382+
return false;
383+
384+
auto removalRange = getRemovalRange(i);
385+
diag.fixItRemoveChars(removalRange.Start, removalRange.End);
386+
return true;
387+
};
388+
366389
// Check all of the types listed in the inheritance clause.
367390
Type superclassTy;
368391
SourceRange superclassRange;
@@ -394,12 +417,12 @@ void TypeChecker::checkInheritanceClause(Decl *decl,
394417
if (inheritedCanTy == Context.TheAnyType) {
395418
if (!isConstraint) {
396419
auto diagLoc = inherited.getSourceRange().Start;
397-
auto removalRange = getRemovalRange(i);
420+
auto diag = diagnose(diagLoc, diag::redundant_conformance_warning,
421+
declInterfaceTy, inheritedTy);
422+
diag.highlight(inherited.getSourceRange());
423+
tryAddRemovalFixIt(diag, i);
424+
diag.flush();
398425

399-
diagnose(diagLoc, diag::redundant_conformance_warning, declInterfaceTy,
400-
inheritedTy)
401-
.highlight(inherited.getSourceRange())
402-
.fixItRemoveChars(removalRange.Start, removalRange.End);
403426
diagnose(diagLoc, diag::all_types_implicitly_conform_to, inheritedTy);
404427
}
405428
continue;
@@ -417,19 +440,19 @@ void TypeChecker::checkInheritanceClause(Decl *decl,
417440
knownType->second.second.Start)
418441
.is(tok::kw_class)) {
419442
SourceLoc classLoc = knownType->second.second.Start;
420-
SourceRange removeRange = getRemovalRange(knownType->second.first);
421443

422-
diagnose(classLoc, diag::duplicate_anyobject_class_inheritance)
423-
.fixItRemoveChars(removeRange.Start, removeRange.End);
444+
auto diag =
445+
diagnose(classLoc, diag::duplicate_anyobject_class_inheritance);
446+
tryAddRemovalFixIt(diag, knownType->second.first);
424447
inherited.setInvalidType(Context);
425448
continue;
426449
}
427450

428-
auto removeRange = getRemovalRange(i);
429-
diagnose(inherited.getSourceRange().Start,
430-
diag::duplicate_inheritance, inheritedTy)
431-
.fixItRemoveChars(removeRange.Start, removeRange.End)
432-
.highlight(knownType->second.second);
451+
auto diag = diagnose(inherited.getSourceRange().Start,
452+
diag::duplicate_inheritance, inheritedTy);
453+
tryAddRemovalFixIt(diag, i);
454+
diag.highlight(knownType->second.second);
455+
433456
inherited.setInvalidType(Context);
434457
continue;
435458
}
@@ -476,11 +499,10 @@ void TypeChecker::checkInheritanceClause(Decl *decl,
476499
if (Context.LangOpts.isSwiftVersion3() && isa<ClassDecl>(decl) &&
477500
inheritedTy->isAnyObject()) {
478501
auto classDecl = cast<ClassDecl>(decl);
479-
auto removeRange = getRemovalRange(i);
480-
diagnose(inherited.getSourceRange().Start,
481-
diag::class_inherits_anyobject,
482-
classDecl->getDeclaredInterfaceType())
483-
.fixItRemoveChars(removeRange.Start, removeRange.End);
502+
auto diag = diagnose(inherited.getSourceRange().Start,
503+
diag::class_inherits_anyobject,
504+
classDecl->getDeclaredInterfaceType());
505+
tryAddRemovalFixIt(diag, i);
484506
continue;
485507
}
486508
}
@@ -498,13 +520,11 @@ void TypeChecker::checkInheritanceClause(Decl *decl,
498520

499521
// If this is not the first entry in the inheritance clause, complain.
500522
if (i > 0) {
501-
auto removeRange = getRemovalRange(i);
502-
503-
diagnose(inherited.getSourceRange().Start,
504-
diag::raw_type_not_first, inheritedTy)
505-
.fixItRemoveChars(removeRange.Start, removeRange.End)
506-
.fixItInsert(inheritedClause[0].getSourceRange().Start,
507-
inheritedTy.getString() + ", ");
523+
auto diag = diagnose(inherited.getSourceRange().Start,
524+
diag::raw_type_not_first, inheritedTy);
525+
if (tryAddRemovalFixIt(diag, i))
526+
diag.fixItInsert(inheritedClause[0].getSourceRange().Start,
527+
inheritedTy.getString() + ", ");
508528

509529
// Fall through to record the raw type.
510530
}
@@ -558,12 +578,11 @@ void TypeChecker::checkInheritanceClause(Decl *decl,
558578

559579
// If this is not the first entry in the inheritance clause, complain.
560580
if (isa<ClassDecl>(decl) && i > 0) {
561-
auto removeRange = getRemovalRange(i);
562-
diagnose(inherited.getSourceRange().Start,
563-
diag::superclass_not_first, inheritedTy)
564-
.fixItRemoveChars(removeRange.Start, removeRange.End)
565-
.fixItInsert(inheritedClause[0].getSourceRange().Start,
566-
inheritedTy.getString() + ", ");
581+
auto diag = diagnose(inherited.getSourceRange().Start,
582+
diag::superclass_not_first, inheritedTy);
583+
if (tryAddRemovalFixIt(diag, i))
584+
diag.fixItInsert(inheritedClause[0].getSourceRange().Start,
585+
inheritedTy.getString() + ", ");
567586

568587
// Fall through to record the superclass.
569588
}

‎test/FixCode/fixits-redundant-any.swift

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// RUN: not %swift -typecheck -target %target-triple %s -fixit-all -emit-fixits-path %t.remap
2+
// RUN: c-arcmt-test %t.remap | arcmt-test -verify-transformed-files %s.result
3+
4+
protocol P {}
5+
6+
// FIX-ME(SR-8102): Emit fix-its for the last requirements once we support bulk
7+
// removal of 'dependant' fix-its.
8+
struct S1<T> where T : Any, T : Any, T : Any {}
9+
struct S2<T> where T : P, T : Any, T : Any {}
10+
struct S3<T> where T : Any, T : P, T : Any {}
11+
struct S4<T> where T : Any, T : Any, T : P {}
12+
struct S5<T> where T : Any {}
13+
14+
protocol P1 {
15+
associatedtype X1 where X1 : Any
16+
associatedtype X2 where X2 : Any, X2 : Any
17+
associatedtype X3 where X2 : Any, X2 : Any, X2 : P
18+
}
19+
20+
// FIX-ME(SR-8102): Always emit fix-its for the second conformances once we
21+
// support bulk removal of 'dependant' fix-its.
22+
struct S6 : Any, Any, Any {}
23+
struct S7 : P, Any, Any {}
24+
struct S8 : Any, P, Any {}
25+
struct S9 : Any, Any, P {}
26+
struct S10 : Any {}
27+
struct S11 : Any, P {}
28+
29+
enum E1 : Any, String { case x }
30+
enum E2 : String, Any { case x }
31+
enum E3 : Any {}
32+
enum E4 : Any, Any, String { case x }
33+
34+
class C {}
35+
class C1 : Any, C {}
36+
class C2 : C, Any {}
37+
class C3 : Any {}
38+
class C4 : Any, Any, C {}
39+
class C5 : Any, AnyObject {}
40+
class C6 : AnyObject, Any {}
41+
class C7 : Any, Any, AnyObject {}
42+
43+
// FIX-ME: Implement redundant Any fix-its for requirements parsed as inheritance clauses.
44+
struct S12<T : Any> {}
45+
protocol P2 : Any {}
46+
protocol P3 : Any, Any {}
47+
protocol P4 : Any, class, AnyObject {} // Parse will re-arrange 'class' to be first.
48+
protocol P5 : class, Any, AnyObject {}
49+
protocol P6 : class, AnyObject, Any {}
50+
51+
protocol P7 {
52+
associatedtype X1 : Any
53+
associatedtype X2 : Any, Any
54+
}

‎test/FixCode/fixits-redundant-any.swift.result

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// RUN: not %swift -typecheck -target %target-triple %s -fixit-all -emit-fixits-path %t.remap
2+
// RUN: c-arcmt-test %t.remap | arcmt-test -verify-transformed-files %s.result
3+
4+
protocol P {}
5+
6+
// FIX-ME(SR-8102): Emit fix-its for the last requirements once we support bulk
7+
// removal of 'dependant' fix-its.
8+
struct S1<T> where T : Any {}
9+
struct S2<T> where T : P, T : Any {}
10+
struct S3<T> where T : P, T : Any {}
11+
struct S4<T> where T : P {}
12+
struct S5<T> {}
13+
14+
protocol P1 {
15+
associatedtype X1
16+
associatedtype X2 where X2 : Any
17+
associatedtype X3 where X2 : P
18+
}
19+
20+
// FIX-ME(SR-8102): Always emit fix-its for the second conformances once we
21+
// support bulk removal of 'dependant' fix-its.
22+
struct S6 : Any {}
23+
struct S7 : P {}
24+
struct S8 : P {}
25+
struct S9 : Any, P {}
26+
struct S10 {}
27+
struct S11 : P {}
28+
29+
enum E1 : String { case x }
30+
enum E2 : String { case x }
31+
enum E3 {}
32+
enum E4 : String, Any { case x }
33+
34+
class C {}
35+
class C1 : C {}
36+
class C2 : C {}
37+
class C3 {}
38+
class C4 : C, Any {}
39+
class C5 : AnyObject {}
40+
class C6 : AnyObject {}
41+
class C7 : Any, AnyObject {}
42+
43+
// FIX-ME: Implement redundant Any fix-its for requirements parsed as inheritance clauses.
44+
struct S12<T : Any> {}
45+
protocol P2 : Any {}
46+
protocol P3 : Any, Any {}
47+
protocol P4 : class, Any, AnyObject {} // Parse will re-arrange 'class' to be first.
48+
protocol P5 : Any, AnyObject {}
49+
protocol P6 : AnyObject, Any {}
50+
51+
protocol P7 {
52+
associatedtype X1 : Any
53+
associatedtype X2 : Any, Any
54+
}

‎test/Generics/redundant_constraints.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,15 @@ struct S07<T> where T : Any, T : P1 {}
2626
// expected-note@-2 {{all types implicitly conform to 'Any'}}
2727

2828
struct S08<T> where T : P1, T : Any {}
29-
// expected-warning@-1 {{redundant conformance constraint 'T': 'Any'}} {{27-36=}}
29+
// expected-warning@-1 {{redundant conformance constraint 'T': 'Any'}} // FIX-ME(SR-8102): Add expected fix-it {{27-36=}}
3030
// expected-note@-2 {{all types implicitly conform to 'Any'}}
3131

3232
struct S09<T> where T : P1, T : Any, T : P2 {}
3333
// expected-warning@-1 {{redundant conformance constraint 'T': 'Any'}} {{29-38=}}
3434
// expected-note@-2 {{all types implicitly conform to 'Any'}}
3535

3636
struct S10<T> where T : P1, T : P2, T : Any {}
37-
// expected-warning@-1 {{redundant conformance constraint 'T': 'Any'}} {{35-44=}}
37+
// expected-warning@-1 {{redundant conformance constraint 'T': 'Any'}} // FIX-ME(SR-8102): Add expected fix-it {{35-44=}}
3838
// expected-note@-2 {{all types implicitly conform to 'Any'}}
3939

4040
struct S11<T, U> {}
@@ -47,7 +47,7 @@ extension S11 where T : Any, T : P1 {} // expected-warning {{redundant conforman
4747
extension S11 where T : Any, U : Any {}
4848
// expected-warning@-1 {{redundant conformance constraint 'T': 'Any'}} {{21-30=}}
4949
// expected-note@-2 {{all types implicitly conform to 'Any'}}
50-
// expected-warning@-3 {{redundant conformance constraint 'U': 'Any'}} {{28-37=}}
50+
// expected-warning@-3 {{redundant conformance constraint 'U': 'Any'}} // FIX-ME(SR-8102): Add expected fix-it {{28-37=}}
5151
// expected-note@-4 {{all types implicitly conform to 'Any'}}
5252

5353

@@ -68,7 +68,7 @@ protocol P4 {
6868
// expected-note@-4 {{all types implicitly conform to 'Any'}}
6969
// expected-warning@-5 {{redundant conformance constraint 'Self.X2': 'Any'}} {{27-37=}}
7070
// expected-note@-6 {{all types implicitly conform to 'Any'}}
71-
// expected-warning@-7 {{redundant conformance constraint 'Self.X2': 'Any'}} {{35-45=}}
71+
// expected-warning@-7 {{redundant conformance constraint 'Self.X2': 'Any'}} // FIX-ME(SR-8102): Add expected fix-it {{35-45=}}
7272
// expected-note@-8 {{all types implicitly conform to 'Any'}}
7373
// expected-warning@-9 {{redundant conformance constraint 'Self.X3': 'Any'}} {{27-37=}}
7474
// expected-note@-10 {{all types implicitly conform to 'Any'}}

‎test/decl/inherit/inherit.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,15 @@ class C : B, A { } // expected-error{{multiple inheritance from classes 'B' and
2020
class D : P, A { } // expected-error{{superclass 'A' must appear first in the inheritance clause}}{{12-15=}}{{11-11=A, }}
2121

2222
// SR-8160
23-
class D1 : Any, A { } // expected-error{{superclass 'A' must appear first in the inheritance clause}}{{15-18=}}{{12-12=A, }}
24-
// expected-warning@-1 {{redundant conformance of 'D1' to 'Any'}}
23+
class D1 : Any, A { } // expected-error{{superclass 'A' must appear first in the inheritance clause}} // We don't emit a fix-it in this case in order to avoid overlapping removal fix-it ranges. However, we emit a removal fix-it for the redundant 'Any', which has the same effect.
24+
// expected-warning@-1 {{redundant conformance of 'D1' to 'Any'}} {{12-17=}}
2525
// expected-note@-2 {{all types implicitly conform to 'Any'}}
2626

2727
class D2 : P & P1, A { } // expected-error{{superclass 'A' must appear first in the inheritance clause}}{{18-21=}}{{12-12=A, }}
2828

2929
@usableFromInline
30-
class D3 : Any, A { } // expected-error{{superclass 'A' must appear first in the inheritance clause}}{{15-18=}}{{12-12=A, }}
31-
// expected-warning@-1 {{redundant conformance of 'D3' to 'Any'}}
30+
class D3 : Any, A { } // expected-error{{superclass 'A' must appear first in the inheritance clause}} // We don't emit a fix-it in this case in order to avoid overlapping removal fix-it ranges. However, we emit a removal fix-it for the redundant 'Any', which has the same effect.
31+
// expected-warning@-1 {{redundant conformance of 'D3' to 'Any'}} {{12-17=}}
3232
// expected-note@-2 {{all types implicitly conform to 'Any'}}
3333

3434
@usableFromInline

‎test/decl/protocol/protocols.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ class X11 : AnyAnyAlias {} // expected-warning {{redundant conformance of 'X11'
110110
struct X12 : Any, Any {}
111111
// expected-warning@-1 {{redundant conformance of 'X12' to 'Any'}} {{14-19=}}
112112
// expected-note@-2 {{all types implicitly conform to 'Any'}}
113-
// expected-warning@-3 {{redundant conformance of 'X12' to 'Any'}} {{17-22=}}
113+
// expected-warning@-3 {{redundant conformance of 'X12' to 'Any'}} // FIX-ME(SR-8102): Add expected fix-it {{17-22=}}
114114
// expected-note@-4 {{all types implicitly conform to 'Any'}}
115115

116116
// Explicit conformance checks (unsuccessful)

0 commit comments

Comments
 (0)