[AST] NFC: Make ExtInfo param Optional<>

[davezarzycki]

While it is very convenient to default the ExtInfo state when creating new function types, it also make the intent unclear to those looking to extend ExtInfo state. For example, did a given call site intend to have the default ExtInfo state or does it just happen to work? This matters a lot because function types are regularly unpacked and rebuilt and it's really easy to accidentally drop ExtInfo state.

[davezarzycki]

@swift-ci please test

[compnerd]

Why the stack allocation rather than inlined into the call? Not that it will make a difference in codegen, but may make the diff easier to read.

[davezarzycki]

For various reasons, I'm not a fan of statements/expressions that overflow a line so this is what felt natural to me. If you feel strongly about this, then I can change the pattern used.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 8180cbe792304aeac892fc7db7450c1d2e750e48

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 8180cbe792304aeac892fc7db7450c1d2e750e48

[varungandhi-apple]

[Not a strong preference but] I think it would be better if there was a single named note (strawman name: [FIXME: describe-why-default-ExtInfo-is-acceptable.]), say near the constructor or something, and other places referred to it by its handle (we have this pattern in a handful of places). Having a FIXME spanning multiple lines takes up more screen space, and given that it might not be a pressing issue/critical to understanding the surrounding code, I think it makes the code less readable.

[xedin]

Defaulting is/was acceptable in all the places in the constraint system since it does want a regular swift escaping function in all of the use sites.

[davezarzycki]

Defaulting is/was acceptable in all the places in the constraint system since it does want a regular swift escaping function in all of the use sites.

But what about the other ExtInfo bits, for example throwing?

I ask also because I have a branch that tries to maintain a "pure" bit but I really doubt that Sema wants to default to impure all of the time. :-/

[xedin]

All these places where a new function type is created have to do with convenience of FunctionType container. Only places where ExtInfo is important - when there is a function type matching on e.g. argument <-> parameter conversions or contextual type conversions - all these take their types from AST, so I wouldn't worry about pure/impure distinction being affected by explicitly created function types. Only place where it might matter is AutoClosureExpr one but even there the default is fine since it's a regular escaping swift function.

[davezarzycki]

All these places where a new function type is created have to do with convenience of FunctionType container. Only places where ExtInfo is important - when there is a function type matching on e.g. argument <-> parameter conversions or contextual type conversions - all these take their types from AST, so I wouldn't worry about pure/impure distinction being affected by explicitly created function types. Only place where it might matter is AutoClosureExpr one but even there the default is fine since it's a regular escaping swift function.

If FunctionType is just being used as a container and the ExtInfo data is unimportant, then it sounds like we should add a "has ExtInfo" bit to AnyFunctionType, and then assert that the bit is set before letting callers extract the ExtInfo data. What do you think of this plan? Or am I missing something?

[davezarzycki]

Hi @xedin – So I tried implementing the "has ExtInfo" bit and – at least for the constraint system – eliding the ExtInfo data mostly works. I haven't tried other subsystems. One problem I ran into that the "Equal" constraint does compare ExtInfo data during type matching, so either the type passed to the constraint must have that data, or the type matching needs to treat "no ExtInfo data" as a wildcard during matching. The latter makes me nervous but maybe I shouldn't be. What should we do?

[xedin]

@davezarzycki I'm curious to see the example where Equal failed that way, do you still have it?

[davezarzycki]

Sure. It's the only one left in CSGen.cpp now that I've implemented the "has ExtInfo" bit.

[xedin]

I think we can replace function use there with FunctionInput, FunctionResult constraints. I'm going to run an experiment and let you know how it goes.

[xedin]

Alright, unfortunately such refactoring wouldn't be as straightforward as it seemed - there are bunch of edge-cases where behavior of patterns in different from regular calls e.g. they allow imploding/exploding arguments and empty inputs of a function e.g. () -> Void are handled as (()) by FunctionInput at the moment. I think we should keep that case in CSGen as explicitly supplying ExtInfo for the time being.

[davezarzycki]

I believe I have addressed all of the feedback to date. Also, by implementing the "has ExtInfo" bit, I was able to remove a few of the FIXME by simply eliding the ExtInfo state entirely. I didn't exhaustively check every FIXME though, so I'd encourage others to try if a specific FIXME bothers them.

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 90711f5d61557cdeadf50da7bead0e7136cbe6da

[davezarzycki]

<unknown>:0: error: compiled module was created by an older version of the compiler; rebuild 'Swift' and try again: /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-main/buildbot_linux/swift-linux-x86_64/lib/swift_static/linux/Swift.swiftmodule/x86_64-unknown-linux-gnu.swiftmodule

@swift-ci please clean test linux

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 90711f5d61557cdeadf50da7bead0e7136cbe6da

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 90711f5d61557cdeadf50da7bead0e7136cbe6da

[davezarzycki]

Let's see if I remember how to do cross-repo testing:

swiftlang/llvm-project#2630

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 90711f5d61557cdeadf50da7bead0e7136cbe6da

[davezarzycki]

swiftlang/llvm-project#2630

@swift-ci please test macos

[davezarzycki]

Hi @xedin and @varungandhi-apple – So I think I've addressed all of the feedback to date and the two pull requests are ready.

[xedin]

If I understand this correctly, based on your finding, the only place where we need to pass ExtInfo directly in the solver would be in CSGen when solving for sub-patterns (I also think we should add a TODO: in there to switch it to use Function{Input, Result} constraints from Equal since all we care about is argument/result matching), the rest could remove // FIXME: and drop explicit ExtInfo use. Is this correct?

[davezarzycki]

I've already dropped explicit ExtInfo data from a few places in the compiler where the test suite still passes. In particular, I've removed ExtInfo data from all but one call site in CSGen.cpp. That being said, I haven't exhaustively checked every FunctionType::get() or GenericFunctionType::get() call site.

I'll add the requested FIXME tomorrow.

[davezarzycki]

I've added the requested TODO.

swiftlang/llvm-project#2630
@swift-ci please test

[davezarzycki]

Ping. I believe I've addressed all of the feedback to date. Might someone please mark this as reviewed? Thanks!

[xedin]

Sema changes LGMT. I don't like all these FIXMEs very much but that shouldn't block anything.

[davezarzycki]

Hi @xedin – Are you approving the whole PR? If not, then who might sign off on the non-sema changes? And just to be clear, this change really helps me with a downstream branch where defaulting the ExtInfo state isn't always reasonable; and it's rarely clear what the upstream intent is. I really appreciate you working with me on this change. Thanks!

[xedin]

I think you can land it (we can always revert it) since the behavior is the same everywhere.