Skip to content

Commit

Permalink
dyno: Fix bugs that were breaking chpl-language-server tests (#26007)
Browse files Browse the repository at this point in the history 
This PR fixes bugs that were causing test failures in
`chpl-language-server`.

Make sure to skip resolving function bodies in `resolveFunction` if the
function does not have one (e.g., for `extern` functions).

If `typedSignatureInitial` is called on a nested function and parent
frames are not present, emit a warning and continue if the parent(s) are
generic instead of returning `nullptr`. Future work should make
`typedSignatureInitial` take the parent function signature instead.

Reviewed by @mppf. Thanks!

TESTING

- [x] `linux64`, `standard`
- [x] `test-chpl-language-server`
  • Loading branch information
@dlongnecke-cray
dlongnecke-cray authored Sep 27, 2024
2 parents d99a4b4 + 5ae1c06 commit d4fbd00
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 29 deletions.
82 changes: 58 additions & 24 deletions frontend/lib/resolution/resolution-queries.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,26 @@ static void checkForParenlessMethodFieldRedefinition(Context* context,
}
}

// Returns 'true' and emits an error if parent frames are not in the RC.
static bool errorIfParentFramesNotPresent(ResolutionContext* rc,
const UntypedFnSignature* usig) {
Context* context = rc->context();

// TODO: More specifically, check if parent frames exist.
bool ret = rc->isEmpty();

// TODO: Becomes a structural issue after we pass in the parent.
if (ret) {
const ID& id = usig->id();
context->error(id, "stack frames for the parent of '%s' are not "
"present, so outer variables in its signature "
"cannot be typed",
usig->name().c_str());
}

return ret;
}

static const TypedFnSignature*
typedSignatureInitialImpl(ResolutionContext* rc,
const UntypedFnSignature* untypedSig) {
Expand All @@ -562,16 +582,26 @@ typedSignatureInitialImpl(ResolutionContext* rc,
parentSignature = sig;
}
} else {
// TODO: Have the user pass the parent signature to make this explicit.
auto parentShape = UntypedFnSignature::get(context, parentFnId);
parentSignature = typedSignatureInitial(rc, parentShape);
}

// The parent signature must exist.
if (!parentSignature) return nullptr;

// All parents need to be concrete for the signature to be evaluated.
// TODO: Change this from a warning to a 'return nullptr' once we pass
// in the parent function signature - in that case it is on the caller
// to make sure they are concrete.
for (auto up = parentSignature; up; up = up->parentFn()) {
if (up->needsInstantiation()) return nullptr;
if (up->needsInstantiation()) {
const ID& id = untypedSig->id();
context->error(id, "One or more parent functions was inferred "
"to be generic while constructing the "
"initial signature of '%s'",
untypedSig->name().c_str());
break;
}
}
}

Expand All @@ -581,9 +611,11 @@ typedSignatureInitialImpl(ResolutionContext* rc,
// visit the formals, but not the return type or body
for (auto formal : fn->formals()) formal->traverse(visitor);

// Give up if we could not type outer variables present in the signature.
if (parentSignature && rc->isEmpty() && !visitor.outerVariables.isEmpty()) {
return nullptr;
if (!visitor.outerVariables.isEmpty()) {
CHPL_ASSERT(parentSignature);

// Outer variables can't be typed without stack frames, so give up.
if (errorIfParentFramesNotPresent(rc, untypedSig)) return nullptr;
}

// now, construct a TypedFnSignature from the result
Expand All @@ -598,7 +630,7 @@ typedSignatureInitialImpl(ResolutionContext* rc,
if (auto whereClause = fn->whereClause()) {
if (needsInstantiation) {
// Visit the where clause for generic nested functions just to collect
// outer variables. TODO: Wrap in speculative block?
// outer variables. TODO: Is this OK or could POI muck with this?
if (parentSignature) whereClause->traverse(visitor);
whereResult = TypedFnSignature::WHERE_TBD;
} else {
Expand All @@ -607,9 +639,11 @@ typedSignatureInitialImpl(ResolutionContext* rc,
}
}

// Give up if we could not type outer variables used in the where clause.
if (parentSignature && rc->isEmpty() && !visitor.outerVariables.isEmpty()) {
return nullptr;
if (!visitor.outerVariables.isEmpty()) {
CHPL_ASSERT(parentSignature);

// Outer variables can't be typed without stack frames, so give up.
if (errorIfParentFramesNotPresent(rc, untypedSig)) return nullptr;
}

checkForParenlessMethodFieldRedefinition(context, fn, visitor);
Expand Down Expand Up @@ -2519,23 +2553,22 @@ resolveFunctionByInfoImpl(ResolutionContext* rc, const TypedFnSignature* sig,
const AstNode* ast = parsing::idToAst(context, id);
const Function* fn = ast->toFunction();
const PoiScope* poiScope = poiInfo.poiScope();
bool isInitializer = sig->isInitializer();
PoiInfo resolvedPoiInfo;
ResolutionResultByPostorderID rr;

if (!fn->body() && !isInitializer) {
CHPL_ASSERT(false && "Should only be called on functions!");
// TODO: Make sure the ID is an extern function specifically.
bool canResolveWithoutAst = parsing::idIsExtern(context, sig->id()) ||
sig->isInitializer();
if (!fn && !canResolveWithoutAst) {
CHPL_ASSERT(false && "Unexpected input to 'resolveFunction'!");
return nullptr;
}

const TypedFnSignature* inputSig = sig;
const TypedFnSignature* finalSig = sig;

auto visitor = isInitializer
? Resolver::createForInitializer(rc, fn, poiScope, inputSig, rr)
: Resolver::createForFunction(rc, fn, poiScope, inputSig, rr);
auto visitor = sig->isInitializer()
? Resolver::createForInitializer(rc, fn, poiScope, sig, rr)
: Resolver::createForFunction(rc, fn, poiScope, sig, rr);

if (isInitializer) {
if (sig->isInitializer()) {
CHPL_ASSERT(visitor.initResolver.get());
auto qt = QualifiedType(QualifiedType::VAR, VoidType::get(context));
visitor.returnType = std::move(qt);
Expand All @@ -2549,12 +2582,14 @@ resolveFunctionByInfoImpl(ResolutionContext* rc, const TypedFnSignature* sig,
return nullptr;
}

const TypedFnSignature* finalSig = sig;

// then, compute the return type if it is not an initializer
if (!isInitializer) {
if (!sig->isInitializer()) {
computeReturnType(visitor);

// else, potentially write out a new initializer signature
} else if (visitor.initResolver) {
} else {
finalSig = visitor.initResolver->finalize();
}

Expand All @@ -2566,14 +2601,14 @@ resolveFunctionByInfoImpl(ResolutionContext* rc, const TypedFnSignature* sig,

// check that throws are handled or forwarded
// TODO: Call for initializers as well, and remove checks in the resolver.
if (!isInitializer) checkThrows(rc, rr, fn);
if (!sig->isInitializer()) checkThrows(rc, rr, fn);

// TODO: can this be encapsulated in a method?
resolvedPoiInfo.swap(visitor.poiInfo);
resolvedPoiInfo.setResolved(true);
resolvedPoiInfo.setPoiScope(nullptr);

CHPL_ASSERT(inputSig == finalSig || isInitializer);
CHPL_ASSERT(sig == finalSig || sig->isInitializer());

auto ret = toOwned(new ResolvedFunction(finalSig,
fn->returnIntent(),
Expand Down Expand Up @@ -2758,7 +2793,6 @@ static const ResolvedFunction*
helpResolveFunction(ResolutionContext* rc, const TypedFnSignature* sig,
const PoiScope* poiScope,
bool skipIfRunning) {

// Forget about any inferred signature (to avoid resolving the
// same function twice when working with inferred 'out' formals)
sig = sig->inferredFrom();
Expand Down
10 changes: 5 additions & 5 deletions frontend/lib/resolution/return-type-inference.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1046,10 +1046,11 @@ static bool helpComputeCompilerGeneratedReturnType(Context* context,

// returns 'true' if it was a case handled here & sets 'result' in that case
// returns 'false' if it needs to be computed with a ResolvedVisitor traversal
static bool helpComputeReturnType(Context* context,
static bool helpComputeReturnType(ResolutionContext* rc,
const TypedFnSignature* sig,
const PoiScope* poiScope,
QualifiedType& result) {
Context* context = rc->context();
const UntypedFnSignature* untyped = sig->untyped();

// TODO: Optimize the order of this case and the isCompilerGenerated case
Expand All @@ -1074,8 +1075,7 @@ static bool helpComputeReturnType(Context* context,

// resolve the return type
ResolutionResultByPostorderID resolutionById;
ResolutionContext rc(context);
auto visitor = Resolver::createForFunction(&rc, fn, poiScope, sig,
auto visitor = Resolver::createForFunction(rc, fn, poiScope, sig,
resolutionById);
retType->traverse(visitor);
result = resolutionById.byAst(retType).type();
Expand Down Expand Up @@ -1146,7 +1146,7 @@ static const QualifiedType& returnTypeQuery(ResolutionContext* rc,
const UntypedFnSignature* untyped = sig->untyped();
QualifiedType result;

bool computed = helpComputeReturnType(context, sig, poiScope, result);
bool computed = helpComputeReturnType(rc, sig, poiScope, result);
if (!computed) {
const AstNode* ast = parsing::idToAst(context, untyped->id());
const Function* fn = ast->toFunction();
Expand Down Expand Up @@ -1232,7 +1232,7 @@ const TypedFnSignature* inferOutFormals(ResolutionContext* rc,

void computeReturnType(Resolver& resolver) {
QualifiedType returnType;
bool computed = helpComputeReturnType(resolver.context,
bool computed = helpComputeReturnType(resolver.rc,
resolver.typedSignature,
resolver.poiScope,
returnType);
Expand Down
21 changes: 21 additions & 0 deletions frontend/test/resolution/testResolve.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,26 @@ static void test26() {
}
}

// Make sure that 'extern' functions still have 'ResolvedFunction' entries.
static void test27() {
Context ctx;
Context* context = &ctx;
ErrorGuard guard(context);

// 'this' qualified, type field
std::string prog =
R"""(
extern proc foo(): int;
)""";

auto m = parseModule(context, prog);
auto f = m->stmt(0)->toFunction();
assert(f && f->linkage() == Decl::EXTERN);
auto rf = resolveConcreteFunction(context, f->id());
assert(rf->returnType().type()->isIntType());
assert(guard.realizeErrors() == 0);
}

int main() {
test1();
test2();
Expand Down Expand Up @@ -1715,6 +1735,7 @@ int main() {
test24();
test25();
test26();
test27();

return 0;
}

0 comments on commit d4fbd00

Please sign in to comment.