Skip to content
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

[CodeComplete] Fix code completion of initialization for variable wrapped by generic property wrapper #36517

Merged
merged 1 commit into from Mar 26, 2021

Conversation

ahoppen
Copy link
Contributor

@ahoppen ahoppen commented Mar 19, 2021

If completing the initialization of a variable that’s wrapped in a generic, unbound property wrapper, the expression's type is an UnboundGenericType. AFAICT that’s expected and the correct type to assign.

We hit the first assertion failure by trying to retrieve that type's substitution map. UnboundGenericTypes were not handled here, so we hit an assertion. AFAICT we can't get any substitutions out of an unbound generic type, so we should just continue looking into the parent type.

We hit the second assertion when trying to retrieve the property wrapper’s backing type, which is null (becuase it couldn't be resolved). However, we haven’t emitted a diagnostic because the variable declaration is perfectly valid. Hence I’m removing the assertion.

Fixes rdar://64141399

@ahoppen ahoppen requested a review from rintaro March 19, 2021 16:08
@ahoppen
Copy link
Contributor Author

ahoppen commented Mar 19, 2021

@swift-ci Please smoke test

@@ -580,7 +580,6 @@ PropertyWrapperBackingPropertyTypeRequest::evaluate(

ASTContext &ctx = var->getASTContext();
Type type = ctx.getSideCachedPropertyWrapperBackingPropertyType(var);
assert(type || ctx.Diags.hadAnyError());
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing this assertion, could you insert

    if (binding->isInvalid())
      return Type();

at the end of if (binding && binding->isInitialized(index)) branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that looks like a cleaner solution. Applied it.

@@ -0,0 +1,32 @@
// RUN: %complete-test %s -tok=COMPLETE_GENERIC
Copy link
Member

Choose a reason for hiding this comment

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

complete-test is SourceKit test. For non-SourceKit specific changes we use swift-ide-test in test/IDE/complete_*.swift. But in this specific case, please use validation-test/IDE/crashers_2_fixed/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn’t know about validation-test/IDE/crashers_2_fixed. Moved it there.


public struct MyStruct {
@GenericWrapper var someProperty = #^COMPLETE_GENERIC^#
@GenericContext<Int>.GenericWrapper var someProperty2 = #^COMPLETE_IN_GENERIC_CONTEXT^#
Copy link
Member

Choose a reason for hiding this comment

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

When testing COMPLETE_IN_GENERIC_CONTEXT, other tokens are just erased from the source so,

   @GenericWrapper var someProperty = #^COMPLETE_GENERIC^#
   @GenericContext<Int>.GenericWrapper var someProperty2 = #^COMPLETE_IN_GENERIC_CONTEXT^#

is (probably) parsed as

@GenericWrapper var someProperty = @GenericContext<Int>.GenericWrapper /* missing ';' */ var someProperty2 = <TOKEN>

To prevent this, please wrap them separate types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Moved them to two different types. Didn’t think about that.

@rintaro
Copy link
Member

rintaro commented Mar 19, 2021

@slavapestov could you review getContextSubstitutions() change?

…pped by generic property wrapper

If completing the initialization of a variable that’s wrapped in a generic, unbound property wrapper, the expression's type is an `UnboundGenericType`. AFAICT that’s expected and the correct type to assign.

We hit the first assertion failure by trying to retrieve that type's substitution map. `UnboundGenericType`s were not handled here, so we crashed. AFAICT we can't get any substitutions out of an unbound generic type, so we should just continue looking into the parent type.

We hit the second assertion when trying to retrieve the property wrapper’s backing type, which is null (becuase it couldn't be resolved). However, we haven’t emitted a diagnostic because the variable declaration is perfectly valid. Hence I’m removing the assertion.

Fixes rdar://64141399
@ahoppen
Copy link
Contributor Author

ahoppen commented Mar 23, 2021

@swift-ci Please smoke test

@ahoppen
Copy link
Contributor Author

ahoppen commented Mar 25, 2021

@swift-ci Please smoke test macOS

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.

None yet

3 participants