Skip to content

[4.0] Diagnose exclusivity violations for noescape closures #10362

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

Conversation

devincoughlin
Copy link
Contributor

This PR enables checking exclusivity violations for no escape closures. For testing purposes it includes all the commits from #10360 plus one additional commit (73d7476) that is the only new content for this PR w/r/t 10360.

Make helper functions static and avoid defining one except when assertions
are enabled.
Relax the static enforcement of exclusive access so that we no longer diagnose
on accesses to separate stored structs of the same property:

takesInout(&s.f1, &s.f2) // no-warning

And perform the analogous relaxation for tuple elements.

To do this, track for each begin_access the projection path from that
access and record the read and write-like modifications on a per-subpath
basis.

We still warn if the there are conflicting accesses on subpaths where one is
the prefix of another.

This commit leaves the diagnostic text in a not-so-good shape since we refer
to the DescriptiveDeclKind of the access even describing a subpath.

I'll fix that up in a later commit that changes only diagnostic text.

https://bugs.swift.org/browse/SR-5119
rdar://problem/31909639
Remove the descriptive decl kind (since with subpaths it is not correct and
cannot represent a tuple element) and change "simultaneous" to "overlapping"
in order to lower the register slightly and avoid connoting threading.

For example, for the following:

   takesTwoInouts(&x.f, &x.f)

the diagnostic will change from

"simultaneous accesses to var 'x.f', but modification requires exclusive access;
consider copying to a local variable"

to

"overlapping accesses to 'x.f', but modification requires exclusive access;
consider copying to a local variable"
Move IndexTrieNode from DeadObjectElimination into its own header. I plan to
use this data structure when diagnosing static violations of exclusive access.
…le args

Add an interprocedural SIL analysis pass that summarizes the accesses that
closures make on their @inout_aliasable captures. This will be used to
statically enforce exclusivity for calls to functions that take noescape
closures.

The analysis summarizes the accesses on each argument independently and
uses the BottomUpIPAnalysis utility class to iterate to a fixed point when
there are cycles in the call graph.

For now, the analysis is not stored-property-sensitive -- that will come in a
later commit.
…ectionPath

IndexTrie is a more light-weight representation and it works well in this case.
This requires recovering the represented sequence from an IndexTrieNode, so
also add a getParent() method.
swiftlang#10310)

Use the AccessSummaryAnalysis to statically enforce exclusive access for
noescape closures passed as arguments to functions.

We will now diagnose when a function is passed a noescape closure that begins
an access on capture when that same capture already has a conflicting access
in progress at the time the function is applied.

The interprocedural analysis is not yet stored-property sensitive (unlike the
intraprocedural analysis), so this will report violations on accesses to
separate stored properties of the same struct.

rdar://problem/32020710
@devincoughlin
Copy link
Contributor Author

@swift-ci Please test

@devincoughlin devincoughlin requested a review from atrick June 18, 2017 15:32
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

Approved for CCC.

@devincoughlin
Copy link
Contributor Author

@swift-ci Please test source compatibility

@najacque
Copy link
Contributor

@swift-ci please clean test

@devincoughlin
Copy link
Contributor Author

CCC Information:

Explanation:
Add stricter static enforcement of exclusive access for variables captured
by no escape closures. With this change, the compiler will diagnose when
a no escape closure that accesses a capture is passed to a function if that
access conflicts with an access that is already in progress at the time the
function is applied.

For example, the compiler will now diagnose statically on the following:

x.mutatingMethod {
x.mutate()
}

This diagnostic a warning in Swift 3.2 and an error in Swift 4 mode.

Note: This change does not distinguish separate stored properties
accessed inside the closure — inside the closure it treats an access
to a struct stored property as an access to the whole. We plan to relax
this in the future, so this change is stricter that what we intend to
eventually enforce. Nonetheless, we want to get this stricter model
into swift-4.0 branch to gauge its impact and determine if additional model
changes are needed.

Scope of Issue:
This is a source-breaking change. In Swift 4.0 it will cause errors for
code that previously compiled. Fortunately this appears rare: there is
only one project in the Swift Source Compatibility suite with violations.
That project (siesta) has 3 violations, all the in the same function.

Radar: rdar://problem/32020710

Risk: This is a medium risk change. It adds new diagnostics (errors in
Swift 4) and a new analysis pass that runs on all noescape
closures that capture variables. The key risks are implementation mistakes
(crashes in the analysis) and the potential for source breakage
in Swift 4 code. We think this source breakage is relatively rare, but it
will happen.

Testing: We've added the usual regression tests and also evaluated on
the source compatibility suite.

@tkremenek tkremenek merged commit ee74cbd into swiftlang:swift-4.0-branch Jun 20, 2017
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