Skip to content

Conversation

@gottesmm
Copy link
Contributor

@gottesmm gottesmm commented May 19, 2023

This PR contains a few commits for 5.9. One is a test update and the other is a
pure rename so I did not include them in the CCC.

Commit 995debb

• Description: [copy-operator] Add support for the copy operator in preparation
for making consuming and borrowing no implicit copy.

Some notes:

  1. I implemented this as a contextual keyword that can only apply directly to
    lvalues. This ensures that we can still call functions called copy, define
    variables named copy, etc. I added tests for both the c++ and swift-syntax based
    parsers to validate this. So there shouldn't be any source breaks.

  2. I did a little bit of type checker work to ensure that we do not treat
    copy_expr's result as an lvalue. Otherwise, one could call mutating functions on
    it or assign to it, which we do not want since the result of copy_value is

  3. As expected, by creating a specific expr, I was able to have much greater
    control of the SILGen codegen and thus eliminate extraneous copies and other
    weirdness than if we used a function and had to go through SILGenApply.

• Risk: Low risk. The only potential risk is that the operator could cause a
source break by breaking parsing for something named copy. But, this was
implemented in such a way (and tests were added) to ensure that these cases
still parse correctly.
• Original PR: #65935
• Reviewed By: @jckarter
• Testing: I added tests to validate that we can still parse variables,
functions, etc named copy. I also added compiler tests for the SILGen code
generation.
• Resolves: rdar://101862423

Commit ed0a9cf

• Description: [consume] Fix consume parsing so we handle code completion and
dollar identifiers correctly. This means we can perform code completion and
one can now consume an anonymous closure argument. I also added one test case
that shows that we properly handle a variable called consume in a switch as
statement. This was handled correctly bt the c++ parser, but not the swift
parser. I put it in the c++ parser test suite here just to make sure that we
continue to handle this correctly.
• Risk: Low risk. Just enables us to parse these two situations.
• Original PR: #65935
• Reviewed By: @jckarter
• Testing: I added tests that validated this behavior.
• Resolves: rdar://109479131

Commit a10a5db

• Description: [consume-operator] Change consume so that its result is not
considered an lvalue.

Before this change it was possible to:

  1. Call mutating methods on a consume result.
  2. assign into a consume (e.x.: var x = ...; (consume x) = value.

From an implementation perspective, this involved just taking the logic I
already used for the CopyExpr and reusing it for ConsumeExpr with some small
tweaks.

The reason to take this is that fixing this in the future will cause a source
break and we know that we do not want to allow for people to do this.

Also, this (as shown by the test update in the next commit), also fixed a
separate bug that caused us to emit a copy of noncopyable type error when
applying consume to a noncopyable var.

• Risk: Low risk. Only affects situations where users call mutating methods on
the result of a consume or attempt to assign into a consume.
• Original PR: #65935
• Reviewed By: @jckarter
• Testing: I added tests that validated that we emit errors when
mutating/assignment the result of a consume and also added some more SILGen
tests to make sure that the change affected code generation in the way we
expected. Importantly, I did not need to touch the diagnostic tests since I
changed the code generation in such a way that the checker still worked. In
the next commit I added a test that validated as well the noncopyable type
behavior that I mentioned above.
• Resolves: rdar://109479440, rdar://109222496

gottesmm added 5 commits May 19, 2023 14:55
…making consuming and borrowing no implicit copy.

Some notes:

1. I implemented this as a contextual keyword that can only apply directly to
lvalues. This ensures that we can still call functions called copy, define
variables named copy, etc. I added tests for both the c++ and swift-syntax based
parsers to validate this. So there shouldn't be any source breaks.

2. I did a little bit of type checker work to ensure that we do not treat
copy_expr's result as an lvalue. Otherwise, one could call mutating functions on
it or assign to it, which we do not want since the result of copy_value is

3. As expected, by creating a specific expr, I was able to have much greater
control of the SILGen codegen and thus eliminate extraneous copies and other
weirdness than if we used a function and had to go through SILGenApply.

rdar://101862423
(cherry picked from commit e6f1691)
…identifiers correctly.

This resulted from explorations around implementing the copy expr.

rdar://109479131
(cherry picked from commit a155209)
…d an lvalue.

Before this change it was possible to:

1. Call mutating methods on a consume result.
2. assign into a consume (e.x.: var x = ...; (consume x) = value.

From an implementation perspective, this involved just taking the logic I
already used for the CopyExpr and reusing it for ConsumeExpr with some small
tweaks.

rdar://109479440
(cherry picked from commit 67fcb1c)
…on var without errors to work.

I found that in the given case, an extra allocation was being emitted causing us
to in the success case to have an extra copy. In the previous commit, as part of
updating the codegen for consume, I also ensured that we wouldn't have that
extra allocation any more by handling the lvalue directly.

rdar://109222496
(cherry picked from commit 2a07d76)
@gottesmm
Copy link
Contributor Author

@gottesmm
Copy link
Contributor Author

Linux test failure is due to a slight difference in enablement flags on 5.9 vs main:

/home/build-user/swift/test/Sema/copy_expr_noimplicit_copy.swift:8:3: error: unexpected error produced: Can not use feature when experimental move only is disabled! Pass the frontend flag -enable-experimental-move-only to swift to enable the usage of this language feature
@_noImplicitCopy let x = Klass()
^

@gottesmm
Copy link
Contributor Author

Evidently no implicit copy on 5.9 is also behind this flag.
@gottesmm gottesmm force-pushed the release-5.9-copy-consume branch from e440154 to 10443d3 Compare May 20, 2023 01:47
@gottesmm
Copy link
Contributor Author

@gottesmm gottesmm merged commit c4e8f67 into swiftlang:release/5.9 May 23, 2023
@gottesmm gottesmm deleted the release-5.9-copy-consume branch May 23, 2023 17:08
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.

3 participants