Skip to content

[Sema] Fix spurious use_local_before_declaration errors. #16967

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 1 commit into from
Jun 27, 2018
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
75 changes: 54 additions & 21 deletions lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,60 @@ resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC) {

// FIXME: Need to refactor the way we build an AST node from a lookup result!

SmallVector<ValueDecl*, 4> ResultValues;
ValueDecl *localDeclAfterUse = nullptr;
auto isValid = [&](ValueDecl *D) {
// FIXME: The source-location checks won't make sense once
// EnableASTScopeLookup is the default.
if (Loc.isValid() && D->getLoc().isValid() &&
D->getDeclContext()->isLocalContext() &&
D->getDeclContext() == DC &&
Context.SourceMgr.isBeforeInBuffer(Loc, D->getLoc())) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is necessary, but not sufficient for all cases. For example, a variable in a guard cannot be used within the else of that guard:

guard let x = someOptional else { /* x is not visible here */ }

However, I think that's okay: this pull request is about improving on the current situation, and as the FIXME notes, ASTScope-based name lookup is the full answer. More importantly, it's not wrong to do what this new code is doing, i.e., it won't reject valid code.

localDeclAfterUse = D;
return false;
}
return true;
};
bool AllDeclRefs = findNonMembers(
*this, Lookup.innerResults(), UDRE->getRefKind(), /*breakOnMember=*/true,
ResultValues, isValid);

// If local declaration after use is found, check outer results for
// better matching candidates.
if (localDeclAfterUse) {
auto innerDecl = localDeclAfterUse;

// Perform a thorough lookup if outer results was not included before.
if (!lookupOptions.contains(NameLookupFlags::IncludeOuterResults)) {
auto option = lookupOptions;
option |= NameLookupFlags::IncludeOuterResults;
Lookup = lookupUnqualified(DC, Name, Loc, option);
}

while (localDeclAfterUse) {
if (Lookup.outerResults().empty()) {
diagnose(Loc, diag::use_local_before_declaration, Name);
diagnose(innerDecl, diag::decl_declared_here, Name);
Expr *error = new (Context) ErrorExpr(UDRE->getSourceRange());
return error;
}

Lookup.shiftDownResults();
ResultValues.clear();
localDeclAfterUse = nullptr;
AllDeclRefs = findNonMembers(
*this, Lookup.innerResults(), UDRE->getRefKind(), /*breakOnMember=*/true,
ResultValues, isValid);
}

// Drop outer results if they are not supposed to be included.
if (!lookupOptions.contains(NameLookupFlags::IncludeOuterResults)) {
Lookup.filter([&](LookupResultEntry Result, bool isOuter) {
return !isOuter;
});
}
}

// If we have an unambiguous reference to a type decl, form a TypeExpr.
if (Lookup.size() == 1 && UDRE->getRefKind() == DeclRefKind::Ordinary &&
isa<TypeDecl>(Lookup[0].getValueDecl())) {
Expand All @@ -586,27 +640,6 @@ resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC) {
UDRE->isImplicit());
}

SmallVector<ValueDecl*, 4> ResultValues;
Expr *error = nullptr;
bool AllDeclRefs = findNonMembers(
*this, Lookup.innerResults(), UDRE->getRefKind(), /*breakOnMember=*/true,
ResultValues, [&](ValueDecl *D) {
// FIXME: The source-location checks won't make sense once
// EnableASTScopeLookup is the default.
if (Loc.isValid() && D->getLoc().isValid() &&
D->getDeclContext()->isLocalContext() &&
D->getDeclContext() == DC &&
Context.SourceMgr.isBeforeInBuffer(Loc, D->getLoc())) {
diagnose(Loc, diag::use_local_before_declaration, Name);
diagnose(D, diag::decl_declared_here, Name);
error = new (Context) ErrorExpr(UDRE->getSourceRange());
return false;
}
return true;
});
if (error)
return error;

if (AllDeclRefs) {
// Diagnose uses of operators that found no matching candidates.
if (ResultValues.empty()) {
Expand Down
20 changes: 20 additions & 0 deletions lib/Sema/TypeCheckNameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,26 @@ void LookupResult::filter(
Results.end());
}

void LookupResult::shiftDownResults() {
// Remove inner results.
Results.erase(Results.begin(), Results.begin() + IndexOfFirstOuterResult);
IndexOfFirstOuterResult = 0;

if (Results.empty())
return;

// Compute IndexOfFirstOuterResult.
const DeclContext *dcInner = Results.front().getValueDecl()->getDeclContext();
for (auto &&result : Results) {
const DeclContext *dc = result.getValueDecl()->getDeclContext();
if (dc == dcInner ||
(dc->isModuleScopeContext() && dcInner->isModuleScopeContext()))
++IndexOfFirstOuterResult;
else
break;
}
}

namespace {
/// Builder that helps construct a lookup result from the raw lookup
/// data.
Expand Down
5 changes: 5 additions & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ class LookupResult {
/// Filter out any results that aren't accepted by the given predicate.
void
filter(llvm::function_ref<bool(LookupResultEntry, /*isOuter*/ bool)> pred);

/// Shift down results by dropping inner results while keeping outer
/// results (if any), the innermost of which are recogized as inner
/// results afterwards.
void shiftDownResults();
};

/// The result of name lookup for types.
Expand Down
8 changes: 4 additions & 4 deletions test/NameBinding/name-binding.swift
Original file line number Diff line number Diff line change
Expand Up @@ -195,16 +195,16 @@ test+++
//===----------------------------------------------------------------------===//

func forwardReference() {
x = 0 // expected-error{{use of local variable 'x' before its declaration}}
var x: Float = 0.0 // expected-note{{'x' declared here}}
v = 0 // expected-error{{use of local variable 'v' before its declaration}}
var v: Float = 0.0 // expected-note{{'v' declared here}}
}

class ForwardReference {
var x: Int = 0

func test() {
x = 0 // expected-error{{use of local variable 'x' before its declaration}}
var x: Float = 0.0 // expected-note{{'x' declared here}}
x = 0
var x: Float = 0.0 // expected-warning{{variable 'x' was never used; consider replacing with '_' or removing it}}
}
}

Expand Down
93 changes: 91 additions & 2 deletions test/Sema/diag_use_before_declaration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,97 @@ func sr5163() {
var foo: Int?

func test() {
guard let bar = foo else { // expected-error {{use of local variable 'foo' before its declaration}}
guard let bar = foo else {
return
}
let foo = String(bar) // expected-note {{'foo' declared here}}
let foo = String(bar)
}

// SR-7660
class C {
var variable: Int?
func f() {
guard let _ = variable else { return }
let variable = 1 // expected-warning {{initialization of immutable value 'variable' was never used; consider replacing with assignment to '_' or removing it}}
}
}

//===----------------------------------------------------------------------===//
// Nested scope
//===----------------------------------------------------------------------===//

func nested_scope_1() {
do {
do {
let _ = x // expected-error {{use of local variable 'x' before its declaration}}
let x = 111 // expected-note {{'x' declared here}}
}
let x = 11
}
let x = 1
}

func nested_scope_2() {
do {
let x = 11
do {
let _ = x
let x = 111 // expected-warning {{initialization of immutable value 'x' was never used; consider replacing with assignment to '_' or removing it}}
}
}
let x = 1 // expected-warning {{initialization of immutable value 'x' was never used; consider replacing with assignment to '_' or removing it}}
}

func nested_scope_3() {
let x = 1
do {
do {
let _ = x
let x = 111 // expected-warning {{initialization of immutable value 'x' was never used; consider replacing with assignment to '_' or removing it}}
}
let x = 11 // expected-warning {{initialization of immutable value 'x' was never used; consider replacing with assignment to '_' or removing it}}
}
}

//===----------------------------------------------------------------------===//
// Type scope
//===----------------------------------------------------------------------===//

class Ty {
var v : Int?

func fn() {
let _ = v
let v = 1 // expected-warning {{initialization of immutable value 'v' was never used; consider replacing with assignment to '_' or removing it}}
}
}

//===----------------------------------------------------------------------===//
// File scope
//===----------------------------------------------------------------------===//

let g = 0

func file_scope_1() {
let _ = g
let g = 1 // expected-warning {{initialization of immutable value 'g' was never used; consider replacing with assignment to '_' or removing it}}
}

func file_scope_2() {
let _ = gg // expected-error {{use of local variable 'gg' before its declaration}}
let gg = 1 // expected-note {{'gg' declared here}}
}

//===----------------------------------------------------------------------===//
// Module scope
//===----------------------------------------------------------------------===//

func module_scope_1() {
let _ = print // Legal use of func print declared in Swift Standard Library
let print = "something" // expected-warning {{initialization of immutable value 'print' was never used; consider replacing with assignment to '_' or removing it}}
}

func module_scope_2() {
let _ = another_print // expected-error {{use of local variable 'another_print' before its declaration}}
let another_print = "something" // expected-note {{'another_print' declared here}}
}