Skip to content

[Distributed] Offer fixit for import Distributed when it is required #72948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions include/swift/AST/DiagnosticEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,9 @@ namespace swift {

static const char *fixItStringFor(const FixItID id);

/// Get the best location where an 'import' fixit might be offered.
SourceLoc getBestAddImportFixItLoc(const Decl *Member) const;

/// Add a token-based replacement fix-it to the currently-active
/// diagnostic.
template <typename... ArgTypes>
Expand Down Expand Up @@ -783,6 +786,9 @@ namespace swift {
return fixItReplaceChars(L, L, "%0", {Str});
}

/// Add a fix-it suggesting to 'import' some module.
InFlightDiagnostic &fixItAddImport(StringRef ModuleName);

/// Add an insertion fix-it to the currently-active diagnostic. The
/// text is inserted immediately *after* the token specified.
InFlightDiagnostic &fixItInsertAfter(SourceLoc L, StringRef Str) {
Expand Down Expand Up @@ -1391,6 +1397,11 @@ namespace swift {
SourceLoc getDefaultDiagnosticLoc() const {
return bufferIndirectlyCausingDiagnostic;
}
SourceLoc getBestAddImportFixItLoc(const Decl *Member,
SourceFile *sourceFile) const;
SourceLoc getBestAddImportFixItLoc(const Decl *Member) const {
return getBestAddImportFixItLoc(Member, nullptr);
}
};

/// Remember details about the state of a diagnostic engine and restore them
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -5436,7 +5436,7 @@ ERROR(conflicting_default_argument_isolation,none,
"default argument cannot be both %0 and %1",
(ActorIsolation, ActorIsolation))
ERROR(distributed_decl_needs_explicit_distributed_import,none,
"%kind0 requires explicit import of Distributed module",
"%kind0 declared without importing module 'Distributed'",
(const ValueDecl *))
NOTE(distributed_func_cannot_overload_on_async_only,none,
"%0 previously declared here, cannot overload distributed methods on effect only", (const ValueDecl *))
Expand Down
69 changes: 69 additions & 0 deletions lib/AST/DiagnosticEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,75 @@ InFlightDiagnostic::fixItReplaceChars(SourceLoc Start, SourceLoc End,
return *this;
}

SourceLoc
DiagnosticEngine::getBestAddImportFixItLoc(const Decl *Member,
SourceFile *sourceFile) const {
auto &SM = SourceMgr;

SourceLoc bestLoc;

auto SF =
sourceFile ? sourceFile : Member->getDeclContext()->getParentSourceFile();
if (!SF) {
return bestLoc;
}

for (auto item : SF->getTopLevelItems()) {
// If we found an import declaration, we want to insert after it.
if (auto importDecl =
dyn_cast_or_null<ImportDecl>(item.dyn_cast<Decl *>())) {
SourceLoc loc = importDecl->getEndLoc();
if (loc.isValid()) {
bestLoc = Lexer::getLocForEndOfLine(SM, loc);
}

// Keep looking for more import declarations.
continue;
}

// If we got a location based on import declarations, we're done.
if (bestLoc.isValid())
break;

// For any other item, we want to insert before it.
SourceLoc loc = item.getStartLoc();
if (loc.isValid()) {
bestLoc = Lexer::getLocForStartOfLine(SM, loc);
break;
}
}

return bestLoc;
}

InFlightDiagnostic &InFlightDiagnostic::fixItAddImport(StringRef ModuleName) {
assert(IsActive && "Cannot modify an inactive diagnostic");
auto Member = Engine->ActiveDiagnostic->getDecl();
SourceLoc bestLoc = Engine->getBestAddImportFixItLoc(Member);

if (bestLoc.isValid()) {
llvm::SmallString<64> importText;

// @_spi imports.
if (Member->isSPI()) {
auto spiGroups = Member->getSPIGroups();
if (!spiGroups.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think somebody more familiar with SPI should take a look at this because it seems like we should mention more than just the first one maybe? /cc @xymus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this from what it was doing in another place that @DougGregor developed for such imports so it's probably ok enough but definitely welcome some more input!

importText += "@_spi(";
importText += spiGroups[0].str();
importText += ") ";
}
}

importText += "import ";
importText += ModuleName;
importText += "\n";

return fixItInsert(bestLoc, importText);
}

return *this;
}

InFlightDiagnostic &InFlightDiagnostic::fixItExchange(SourceRange R1,
SourceRange R2) {
assert(IsActive && "Cannot modify an inactive diagnostic");
Expand Down
5 changes: 3 additions & 2 deletions lib/Basic/SourceLoc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@
//
//===----------------------------------------------------------------------===//

#include "swift/Basic/Range.h"
#include "swift/Basic/SourceLoc.h"
#include "swift/AST/SourceFile.h"
#include "swift/Basic/Range.h"
#include "swift/Basic/SourceManager.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"

using namespace swift;

Expand Down
30 changes: 2 additions & 28 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6195,34 +6195,8 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
Member->getDescriptiveKind(), Member->getName(),
definingModule->getName());

auto enclosingSF = getDC()->getParentSourceFile();
SourceLoc bestLoc;
SourceManager &srcMgr = Member->getASTContext().SourceMgr;
for (auto item : enclosingSF->getTopLevelItems()) {
// If we found an import declaration, we want to insert after it.
if (auto importDecl =
dyn_cast_or_null<ImportDecl>(item.dyn_cast<Decl *>())) {
SourceLoc loc = importDecl->getEndLoc();
if (loc.isValid()) {
bestLoc = Lexer::getLocForEndOfLine(srcMgr, loc);
}

// Keep looking for more import declarations.
continue;
}

// If we got a location based on import declarations, we're done.
if (bestLoc.isValid())
break;

// For any other item, we want to insert before it.
SourceLoc loc = item.getStartLoc();
if (loc.isValid()) {
bestLoc = Lexer::getLocForStartOfLine(srcMgr, loc);
break;
}
}

SourceLoc bestLoc =
getBestAddImportFixItLocation(Member, getDC()->getParentSourceFile());
if (bestLoc.isValid()) {
llvm::SmallString<64> importText;

Expand Down
6 changes: 6 additions & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ class FailureDiagnostic {

ConstraintLocator *getLocator() const { return Locator; }

SourceLoc getBestAddImportFixItLocation(const Decl *Member,
SourceFile *sourceFile) const {
auto &engine = Member->getASTContext().Diags;
return engine.getBestAddImportFixItLoc(Member, sourceFile);
}

Type getType(ASTNode node, bool wantRValue = true) const;

/// Get type associated with a given ASTNode without resolving it,
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/TypeCheckDistributed.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ DistributedModuleIsAvailableRequest::evaluate(Evaluator &evaluator,
auto DistributedModule = C.getLoadedModule(C.Id_Distributed);
if (!DistributedModule) {
decl->diagnose(diag::distributed_decl_needs_explicit_distributed_import,
decl);
decl)
.fixItAddImport("Distributed");
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
actor SomeActor {}

distributed actor DA {}
// expected-error@-1{{distributed actor 'DA' requires explicit import of Distributed module}}
// expected-error@-1{{distributed actor 'DA' declared without importing module 'Distributed'}}

distributed actor class DAC {}
// expected-error@-1{{distributed' can only be applied to 'actor' definitions, and distributed actor-isolated async functions}}
Expand All @@ -18,23 +18,23 @@ actor A {
}

actor B {
distributed var neverOk: String { // expected-error{{distributed property 'neverOk' requires explicit import of Distributed module}}
distributed var neverOk: String { // expected-error{{distributed property 'neverOk' declared without importing module 'Distributed'}}
""
}
}

// expected-error@+1{{distributed actor 'DA2' requires explicit import of Distributed module}}
// expected-error@+1{{distributed actor 'DA2' declared without importing module 'Distributed'}}
distributed actor DA2 {

func normal() async {}

// expected-error@+1{{distributed instance method 'dist()' requires explicit import of Distributed module}}
// expected-error@+1{{distributed instance method 'dist()' declared without importing module 'Distributed'}}
distributed func dist() {}

// expected-error@+1{{distributed instance method 'distAsync()' requires explicit import of Distributed module}}
// expected-error@+1{{distributed instance method 'distAsync()' declared without importing module 'Distributed'}}
distributed func distAsync() async {}

// expected-error@+1{{distributed property 'neverOk' requires explicit import of Distributed module}}
// expected-error@+1{{distributed property 'neverOk' declared without importing module 'Distributed'}}
distributed var neverOk: String {
""
}
Expand Down
3 changes: 1 addition & 2 deletions test/Distributed/distributed_missing_import.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
actor SomeActor { }

distributed actor MissingImportDistributedActor_0 { }
// expected-error@-1{{distributed actor 'MissingImportDistributedActor_0' requires explicit import of Distributed module}}
// expected-error@-1{{distributed actor 'MissingImportDistributedActor_0' declared without importing module 'Distributed'}}{{5:1-1=import Distributed\n}}

let t: DistributedActorSystem // expected-error{{cannot find type 'DistributedActorSystem' in scope}}
let a: ActorAddress // expected-error{{cannot find type 'ActorAddress' in scope}}

13 changes: 13 additions & 0 deletions test/Distributed/distributed_missing_import_fixit.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// RUN: %target-typecheck-verify-swift -disable-availability-checking
// REQUIRES: concurrency
// REQUIRES: distributed

import Swift // just any import so we can anchor the fixit onto it

actor SomeActor { }

distributed actor MissingImportDistributedActor_0 { }
// expected-error@-1{{distributed actor 'MissingImportDistributedActor_0' declared without importing module 'Distributed'}}{{6:1-1=import Distributed\n}}

let t: DistributedActorSystem // expected-error{{cannot find type 'DistributedActorSystem' in scope}}