Skip to content

[AST] Improve ErrorType handling in TypeBase::getMemberSubstitutions() #272

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
Dec 11, 2015

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Dec 6, 2015

Historical note: a similar change was made in 0033bda.

Fixes 78 compiler_crashers. ⛵

@lattner
Copy link
Contributor

lattner commented Dec 6, 2015

This LGTM, but I think @DougGregor should also take a look when he gets a chance. Great work, thank you!

-Chris

@AlexDenisov
Copy link
Contributor

Indeed it solves some issues, but the problem actually caused by this line:

baseTy = baseTy->castTo<NominalType>()->getParent();

The best way to fix those crashes, imho, would be to use safer API for casting:

if (const NominalType *NominalTy = baseTy->getAs<NominalType>()) {
  baseTy = NominalTy->getParent();
} else {
  baseTy = nullptr;
}

otherwise some other non-NominalType and non-ErrorType will break the compiler.

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 6, 2015

@AlexDenisov thanks for the input. I would assume the castTo<NominalType> is being done for good reason (i.e. is safe because of other invariants upheld by the AST code), but I don't have enough knowledge to say for sure. @DougGregor will need to confirm.

Really makes me wish this were all written in Swift, so we could use optional chaining and throws 😄

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 6, 2015

Updated with new fixed crashers.

@@ -2498,7 +2498,7 @@ TypeSubstitutionMap TypeBase::getMemberSubstitutions(DeclContext *dc) {

// Gather all of the substitutions for all levels of generic arguments.
GenericParamList *curGenericParams = dc->getGenericParamsOfContext();
while (baseTy) {
while (baseTy && !baseTy->is<ErrorType>()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather that the ErrorType check happen a little later, e.g., turn the "castAs()" into a "getAs()" within an "if". Then, we only check the ErrorType condition if all else fails, and can null out baseTy before breaking out of the loop.

@jtbandes jtbandes force-pushed the fix3 branch 2 times, most recently from 15b222c to bdaf1de Compare December 7, 2015 11:36
@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

@DougGregor: Updated. (Note that some tests are failing for other reasons; I fixed them in #296.)

I have general questions about the error-handling mechanisms throughout; I sent an email to swift-dev entitled "Problems caused by is", and I also filed https://bugs.swift.org/browse/SR-86. I'd be curious to hear your thoughts.

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 9, 2015

@DougGregor please let me know if there's anything else to be done here!

@@ -2517,7 +2517,17 @@ TypeSubstitutionMap TypeBase::getMemberSubstitutions(DeclContext *dc) {
}

// Continue looking into the parent.
baseTy = baseTy->castTo<NominalType>()->getParent();
auto nominalTy = baseTy->getAs<NominalType>();
if (!nominalTy || nominalTy->is<ErrorType>()) {
Copy link
Member

Choose a reason for hiding this comment

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

The getAs will either produce a NominalType or null; there's no need to check for ErrorType here.

DougGregor added a commit that referenced this pull request Dec 11, 2015
[AST] Improve ErrorType handling in TypeBase::getMemberSubstitutions()
@DougGregor DougGregor merged commit 851f2ea into swiftlang:master Dec 11, 2015
@jtbandes jtbandes deleted the fix3 branch December 11, 2015 23:14
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
Revert "Also xfail NonEmpty on swift-4.2-branch"
DougGregor pushed a commit to DougGregor/swift that referenced this pull request Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants