Skip to content

Commit

Permalink
LSP: fix hovering over a string (#26177)
Browse files Browse the repository at this point in the history 
Updates to the Dyno resolver have exposed some bugs that once again
break hovering over `string` with the resolver enabled. This PR fixes a
number of minor issues that cause crashes. The changes are as follows:

1. **Handle type query domains.** Thus, `[?D]` now resolves to a generic
array type as opposed to 'Unknown'.
2. **Treat passing to unknown type as instantiation.** This is important
for cases in which type queries need to be populated. They are only
populated from substitutions, but substitutions are not created for
completely unknown formals, because prior to this change, a completely
unknown formal can be passed to without instantiating (i.e., without
substitution).
3. **Reject candidates that fail due to inability to infer type
queries.** After computing a formal type, we traverse the formal AST to
find out what the type queries are. This may fail (e.g., if the formal
AST doesn't match up with the instantiated type). Thus, the post-query
type will be unknown. This is an error, since it indicates that the type
queries couldn't be properly made sense of and incorporated into the
final type info. In such cases, reject the candidate.
* A concrete example: formal ?k*?t, actual int. The initial type is
unknown; after instantiation, the formal type is int. When resolving
queries, we can't figure out how to spit int across ?k*?t, leaving type
queries unknown. When re-combining the formal type with query info,
?k*?t is again unknown since the queries were not known. This means we
couldn't instantiate ?k*?t with int, and the candidate is rejected.
4. **Handle unknown actuals (needed for out-formals) in builtin
functions.** We have some builtin functions like type/param casting to
string, handled by the compiler. They did not expect `null` actual
types, but this can happen sometimes (e.g., when the call site allows
for the possibility of `out`-formals being matched with the actual).
This change adjusts the code to handle that by simply not attempting to
resolve builtins in that case (none of the have `out` formals, so this
is sound).
5. **Skip unknown arguments when building ranges.** Resolving ranges
boils down to resolving a function call. Dyno skips resolving normal
function calls under certain conditions (e.g., if the argument types
could not be inferred). However, it doesn't do so for ranges, which made
it possible for 'Unknown' or otherwise invalid actuals to slip into the
call resolution process. This commit adjusts the resolver to re-use the
skipping logic.

Reviewed by @dlongnecke-cray -- thanks!

## Testing

- [x] dyno tests
- [x] CLS tests
  • Loading branch information
@DanilaFe
DanilaFe authored Oct 30, 2024
2 parents e75e707 + 9d0d5e5 commit 7ba4bc7
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 93 deletions.
221 changes: 129 additions & 92 deletions frontend/lib/resolution/Resolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3768,6 +3768,89 @@ void Resolver::exit(const TupleDecl* decl) {
exitScope(decl);
}

static SkipCallResolutionReason
shouldSkipCallResolution(Resolver* rv, const uast::AstNode* callLike,
std::vector<const uast::AstNode*> actualAsts,
const CallInfo& ci) {
Context* context = rv->context;
SkipCallResolutionReason skip = NONE;
auto& byPostorder = rv->byPostorder;

if (callLike->isTuple()) return skip;

int actualIdx = 0;
for (const auto& actual : ci.actuals()) {
ID toId; // does the actual refer directly to a particular variable?
const AstNode* actualAst = actualAsts[actualIdx];
if (actualAst != nullptr && byPostorder.hasAst(actualAst)) {
toId = byPostorder.byAst(actualAst).toId();
}
QualifiedType qt = actual.type();
const Type* t = qt.type();

auto formalAst = toId.isEmpty() ? nullptr : parsing::idToAst(context, toId);
bool isNonOutFormal = formalAst != nullptr &&
formalAst->isFormal() &&
formalAst->toFormal()->intent() != Formal::Intent::OUT;

if (t != nullptr && t->isErroneousType()) {
// always skip if there is an ErroneousType
skip = ERRONEOUS_ACT;
} else if (!toId.isEmpty() && !isNonOutFormal &&
qt.kind() != QualifiedType::PARAM &&
qt.kind() != QualifiedType::TYPE &&
qt.isRef() == false) {
// don't skip because it could be initialized with 'out' intent,
// but not for non-out formals because they can't be split-initialized.
} else if (actualAst->isTypeQuery() && ci.calledType().isType()) {
// don't skip for type queries in type constructors
} else {
if (qt.isParam() && qt.param() == nullptr) {
skip = UNKNOWN_PARAM;
} else if (qt.isUnknown()) {
skip = UNKNOWN_ACT;
} else if (t != nullptr && qt.kind() != QualifiedType::INIT_RECEIVER) {
// For initializer calls, allow generic formals using the above
// condition; this way, 'this.init(..)' while 'this' is generic
// should be fine.

auto g = getTypeGenericity(context, t);
bool isBuiltinGeneric = (g == Type::GENERIC &&
(t->isAnyType() || t->isBuiltinType()));
if (qt.isType() && isBuiltinGeneric && rv->substitutions == nullptr) {
skip = GENERIC_TYPE;
} else if (!qt.isType() && g != Type::CONCRETE) {
skip = GENERIC_VALUE;
}
}
}

// Don't skip for type constructors, except due to unknown params.
if (skip != UNKNOWN_PARAM && ci.calledType().isType()) {
skip = NONE;
}

// Do not skip primitive calls that accept a generic type, since they
// may be valid.
if (skip == GENERIC_TYPE && callLike->toPrimCall()) {
skip = NONE;
}

if (skip) {
break;
}
actualIdx++;
}

// Don't try to resolve calls to '=' until later
if (ci.isOpCall() && ci.name() == USTR("=")) {
skip = OTHER_REASON;
}

return skip;
}


bool Resolver::enter(const Range* range) {
return true;
}
Expand Down Expand Up @@ -3798,23 +3881,37 @@ void Resolver::exit(const Range* range) {
const char* function = functions[boundType];

std::vector<CallInfoActual> actuals;
std::vector<const AstNode*> actualAsts;
if (range->lowerBound()) {
actuals.emplace_back(/* type */ byPostorder.byAst(range->lowerBound()).type(),
/* byName */ UniqueString());
actualAsts.push_back(range->lowerBound());
}
if (range->upperBound()) {
actuals.emplace_back(/* type */ byPostorder.byAst(range->upperBound()).type(),
/* byName */ UniqueString());
actualAsts.push_back(range->upperBound());
}


auto ci = CallInfo(/* name */ UniqueString::get(context, function),
/* calledType */ QualifiedType(),
/* isMethodCall */ false,
/* hasQuestionArg */ false,
/* isParenless */ false, actuals);
auto scope = scopeStack.back();
auto inScopes = CallScopeInfo::forNormalCall(scope, poiScope);
auto c = resolveGeneratedCall(context, range, ci, inScopes);
handleResolvedCall(byPostorder.byAst(range), range, ci, c);

// Skip calls when the bounds are unknown to avoid putting function resolution
// into an awkward position.
auto skip = shouldSkipCallResolution(this, range, actualAsts, ci);
if (!skip) {
auto scope = scopeStack.back();
auto inScopes = CallScopeInfo::forNormalCall(scope, poiScope);
auto c = resolveGeneratedCall(context, range, ci, inScopes);
handleResolvedCall(byPostorder.byAst(range), range, ci, c);
} else {
auto& r = byPostorder.byAst(range);
r.setType(QualifiedType());
}
}

bool Resolver::enter(const uast::Domain* decl) {
Expand Down Expand Up @@ -3847,8 +3944,34 @@ void Resolver::exit(const uast::Domain* decl) {

// Add key or range actuals
std::vector<CallInfoActual> actuals;
bool freshDomainQuery = false;
for (auto expr : decl->exprs()) {
actuals.emplace_back(byPostorder.byAst(expr).type(), UniqueString());
auto exprType = byPostorder.byAst(expr).type();

// If it's a type query, we may be looking at [?D] (where a Domain node
// is implicitly created in the array expression AST). In that case,
// we want the fully generic domain type.
if (expr->isTypeQuery() && exprType.type() && exprType.type()->isAnyType()) {
freshDomainQuery = true;
break;
}

actuals.emplace_back(exprType, UniqueString());
}

if (freshDomainQuery) {
if (decl->numExprs() > 1 || decl->usedCurlyBraces()) {
// We can only query the whole domain using a type query, so reject
// the domain expression.
context->error(decl, "cannot query part of a domain");
}

auto& re = byPostorder.byAst(decl);
auto dt = QualifiedType(QualifiedType::CONST_VAR, genericDomainType);
re.setType(dt);

// No need to perform the call to chpl__buildDomainExpr etc.
return;
}

// Add definedConst actual if appropriate
Expand Down Expand Up @@ -3993,89 +4116,6 @@ static const Type* getGenericType(Context* context, const Type* recv) {
return gen;
}

static SkipCallResolutionReason
shouldSkipCallResolution(Resolver* rv, const uast::Call* call,
std::vector<const uast::AstNode*> actualAsts,
ID moduleScopeId,
const CallInfo& ci) {
Context* context = rv->context;
SkipCallResolutionReason skip = NONE;
auto& byPostorder = rv->byPostorder;

if (call->isTuple()) return skip;

int actualIdx = 0;
for (const auto& actual : ci.actuals()) {
ID toId; // does the actual refer directly to a particular variable?
const AstNode* actualAst = actualAsts[actualIdx];
if (actualAst != nullptr && byPostorder.hasAst(actualAst)) {
toId = byPostorder.byAst(actualAst).toId();
}
QualifiedType qt = actual.type();
const Type* t = qt.type();

auto formalAst = toId.isEmpty() ? nullptr : parsing::idToAst(context, toId);
bool isNonOutFormal = formalAst != nullptr &&
formalAst->isFormal() &&
formalAst->toFormal()->intent() != Formal::Intent::OUT;

if (t != nullptr && t->isErroneousType()) {
// always skip if there is an ErroneousType
skip = ERRONEOUS_ACT;
} else if (!toId.isEmpty() && !isNonOutFormal &&
qt.kind() != QualifiedType::PARAM &&
qt.kind() != QualifiedType::TYPE &&
qt.isRef() == false) {
// don't skip because it could be initialized with 'out' intent,
// but not for non-out formals because they can't be split-initialized.
} else if (actualAst->isTypeQuery() && ci.calledType().isType()) {
// don't skip for type queries in type constructors
} else {
if (qt.isParam() && qt.param() == nullptr) {
skip = UNKNOWN_PARAM;
} else if (qt.isUnknown()) {
skip = UNKNOWN_ACT;
} else if (t != nullptr && qt.kind() != QualifiedType::INIT_RECEIVER) {
// For initializer calls, allow generic formals using the above
// condition; this way, 'this.init(..)' while 'this' is generic
// should be fine.

auto g = getTypeGenericity(context, t);
bool isBuiltinGeneric = (g == Type::GENERIC &&
(t->isAnyType() || t->isBuiltinType()));
if (qt.isType() && isBuiltinGeneric && rv->substitutions == nullptr) {
skip = GENERIC_TYPE;
} else if (!qt.isType() && g != Type::CONCRETE) {
skip = GENERIC_VALUE;
}
}
}

// Don't skip for type constructors, except due to unknown params.
if (skip != UNKNOWN_PARAM && ci.calledType().isType()) {
skip = NONE;
}

// Do not skip primitive calls that accept a generic type, since they
// may be valid.
if (skip == GENERIC_TYPE && call->toPrimCall()) {
skip = NONE;
}

if (skip) {
break;
}
actualIdx++;
}

// Don't try to resolve calls to '=' until later
if (ci.isOpCall() && ci.name() == USTR("=")) {
skip = OTHER_REASON;
}

return skip;
}

static const bool& warnForMissingIterKindEnum(Context* context,
const AstNode* astForErr) {
QUERY_BEGIN(warnForMissingIterKindEnum, context, astForErr);
Expand Down Expand Up @@ -4282,10 +4322,7 @@ void Resolver::handleCallExpr(const uast::Call* call) {
CallScopeInfo::forNormalCall(scope, poiScope) :
CallScopeInfo::forQualifiedCall(context, moduleScopeId, scope, poiScope);

auto skip = shouldSkipCallResolution(this, call, actualAsts,
moduleScopeId,
ci);

auto skip = shouldSkipCallResolution(this, call, actualAsts, ci);
if (!skip) {
ResolvedExpression& r = byPostorder.byAst(call);
QualifiedType receiverType = methodReceiverType();
Expand Down
2 changes: 1 addition & 1 deletion frontend/lib/resolution/can-pass.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -993,7 +993,7 @@ CanPassResult CanPassResult::canPass(Context* context,
// when computing an initial candidate, 'b' is unknown
// but we should allow passing an argument to it.
if (formalT->isUnknownType() && !actualQT.isType()) {
return passAsIs();
return instantiate();
}

// allow unknown qualifier for any type actuals
Expand Down
17 changes: 17 additions & 0 deletions frontend/lib/resolution/resolution-queries.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2225,6 +2225,14 @@ ApplicabilityResult instantiateSignature(ResolutionContext* rc,
r.byAst(entry.formal()).setType(formalType);
}

// We've set up the type queries and re-traversed the formal AST to
// compute the type using these queries. If the formal type is still
// unknown at this point, we couldn't extract the type queries, which
// means the call is ill-formed.
if (qFormalType.isUnknownKindOrType()) {
return ApplicabilityResult::failure(sig, FAIL_CANNOT_INSTANTIATE, entry.formalIdx());
}

auto checkType = !useType.isUnknown() ? useType : formalType;
// With the type and query-aware type known, make sure that they're compatible
auto passResult = canPass(context, checkType, qFormalType);
Expand Down Expand Up @@ -3609,6 +3617,15 @@ static bool resolveFnCallSpecial(Context* context,
// TODO: .borrow()
// TODO: chpl__coerceCopy

// Sometimes, actual types can be unknown since we are checking for 'out'
// intent. No special functions here use the 'out' intent, so in this case,
// return false.
for (auto& actual : ci.actuals()) {
if (actual.type().isUnknown()) {
return false;
}
}

// special casts including explicit param casts are resolved here
if (ci.isOpCall() && ci.name() == USTR(":")) {
auto srcQt = ci.actual(0).type();
Expand Down

0 comments on commit 7ba4bc7

Please sign in to comment.