Skip to content

[AST] ParamDecl::isNonEphemeral() not to evaluate InterfaceTypeRequest #32553

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 2 commits into from
Jun 26, 2020

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Jun 25, 2020

-dump-parse for the following code used to hit an assertion failure:

  struct MyStruct {}
  _ = { (val: MyStruct) in }

Because isNonEphemeral() on val parameter unintentionally causes InterfaceTypeRequest::evaluate(). Since isNonEphemeral() inference based on the interface type is not possible after type checking, return false unless hasInterfaceType(). Use the presence of @_nonEphemeral attribute in ASTDumper instead.

Also:

  • Dump parameter list on EnumElementDecl as well
  • Adjust the position of range=... in dumping parameter list

https://bugs.swift.org/browse/SR-12728 / rdar://problem/62895098

@rintaro
Copy link
Member Author

rintaro commented Jun 25, 2020

@swift-ci Please smoke test

@rintaro rintaro requested a review from hamishknight June 25, 2020 20:46
@hamishknight
Copy link
Contributor

Rather than adding a check for hasInterfaceType(), could we change the ASTDumper to check for the presence of the @_nonEphemeral attribute instead of calling isNonEphemeral()?

@CodaFi
Copy link
Contributor

CodaFi commented Jun 25, 2020

Seconded. The semantic versions of these entrypoints are still needed, they just need to be documented as semantic entrypoints.

@rintaro
Copy link
Member Author

rintaro commented Jun 25, 2020

Is isNonEphemeral() only intended to be called after type checking the ParamDecl?
I don't completely understand the semantics of it, but it is called from ConstraintGenerator::inferClosureType() via ParamDecl::toFunctionParam(). Since the ParamDecl of the closure hasn't been type checked yet, getInterfaceType() results ErrorType unless it has TypeRepr. I don't think it's semantically correct. Should we do something for that?

@CodaFi
Copy link
Contributor

CodaFi commented Jun 25, 2020

Aha, that's just closure parameters being their special weird selves. @hamishknight Is it possible to early-exit for un-annotated closure parameters?

@hamishknight
Copy link
Contributor

Ah yeah, it seems we should keep the new arrangement of the logic where we check whether the parent is an EnumElementDecl before querying the interface type, but remove the check for hasInterfaceType.

@rintaro
Copy link
Member Author

rintaro commented Jun 25, 2020

Okay, I will

  • Remove the hasInterfaceType() check in isNonEphemeral().
  • In ASTDumper, use @_nonEphemeral attribute instead of isNonEphemeral()

How do you think of the assertion I added in setNonEphemeralIfPossible()?

@hamishknight
Copy link
Contributor

Sounds good, thank you!

How do you think of the assertion I added in setNonEphemeralIfPossible()?

I'm okay with keeping for it for now, at some point I'd like to refactor away setNonEphemeralIfPossible and have an IsNonEphemeralRequest that can check if the param is a part of an implicit memberwise initializer.

'-dump-parse' for the following code used to hit an assertion failure:

  struct MyStruct {}
  _ = { (val: MyStruct) in }

Because 'isNonEphemeral()' on 'val' parameter unintentionally causes
'InterfaceTypeRequest::evaluate()'. Since 'isNonEphemeral()' inference
based on the interface type is not possible after type checking, return
'false' unless 'hasInterfaceType()'

Also:
  * Dump parameter list on 'EnumElementDecl' as well
  * Adjust the position of 'range=...' in parameter list dumping
@rintaro rintaro force-pushed the ast-isnonephemeral-interfacetype branch from cdf0713 to 9044aa0 Compare June 25, 2020 22:11
@rintaro
Copy link
Member Author

rintaro commented Jun 25, 2020

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Jun 25, 2020

@swift-ci Please smoke test

@rintaro rintaro merged commit 1d20ddc into swiftlang:master Jun 26, 2020
@rintaro rintaro deleted the ast-isnonephemeral-interfacetype branch June 26, 2020 01:41
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