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

[SR-11700] Diagnose exclusivity violations with Dictionary.subscript._modify #54108

Closed
atrick opened this issue Nov 2, 2019 · 7 comments
Closed
Assignees

Comments

@atrick
Copy link
Member

atrick commented Nov 2, 2019

Previous ID SR-11700
Radar None
Original Reporter @atrick
Type Bug
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug
Assignee @atrick
Priority Medium

md5: a2d9c4d922a9abe7ea94e9715dfcbc10

Issue Description:

It appears to be common practice to violate exclusivity using `Dictionary.subscript._modify` in a way that the compiler fails to diagnose.

I'm not too concerned about miscompiles yet. But I don't know how to plug this hole before it becomes bigger without possibly breaking a bunch of source. And I don't know how developers should fix their code without sticking silly 'let's in that defeat the purpose of the 'autoclosure'.

Exclusivity violations could also become apparent after inlining if we don't change anything.

It seems common to access a dictionary that is a struct stored property, and reference the same struct, or one of its members in the `defaultValue:` autoclosure.

In most cases, the violation should be harmless, ignoring possible compiler optimization. On the other hand, if we don't diagnose it, then the user could reentrantly access the dictionary in the middle of key insertion...

public struct Value {
 var i: Int
 mutating func inc() { i += 1 }
}
public struct S {
 var d: [Int: Value] = [:]
 func getValue() -> Value { return Value(i:0) }
 public mutating func addInt(_ k: Int) {
  // self.getValue accessed during self.d modify.
  d[k, default: getValue()].inc()
 }
}

Here is an equivalent exclusivity violation that does not use coroutines and which we do enforce:

public struct Value {
 var i: Int
 mutating func inc(_ f: @autoclosure ()->Int) { i += f() }
}
 public struct S {
 var v: Value
 func getValue() -> Int { return 1 }
 public mutating func addInt(_ k: Int) { v.inc(getValue()) }
}

A PR that fixes diagnostics and breaks a bunch of code:
#28022

@atrick
Copy link
Member Author

atrick commented Nov 2, 2019

@rjmccall any suggestions for fixing this without breaking too much source, or giving developers another alternative?

@rjmccall
Copy link
Member

rjmccall commented Nov 4, 2019

Nothing comes to mind. We can't fall back on dynamic enforcement since methods like getValue will be compiled on the premise that struct members don't need that. In general, I think we just have to acknowledge this as a bug that we have little choice but to fix. Users will need to factor the autoclosure in a way that doesn't need all of self; in your example, they could make getValue() a static method.

@rjmccall
Copy link
Member

rjmccall commented Nov 4, 2019

I can raise that question to the Core Team, though.

@atrick
Copy link
Member Author

atrick commented Nov 4, 2019

I don't see any way forward without breaking source. If the question boils down to how much code will break, I can start working through any cases I find in the SCK.

@rjmccall
Copy link
Member

rjmccall commented Nov 4, 2019

Everyone from the Core Team who's weighed in so far agrees that we should just treat this as a bug fix and accept the source compatibility break. We just need to document it properly in release notes.

@atrick
Copy link
Member Author

atrick commented May 2, 2020

Fixed in swift 5.3

PR: #28022

@atrick
Copy link
Member Author

atrick commented May 2, 2020

Also fixes <rdar://problem/56378713> Investigate why AccessSummaryAnalysis is crashing

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants