Conversation
jesyspa
left a comment
There was a problem hiding this comment.
This looks like a good start, some feedback on the Kotlin, and some on the expected test results.
...ugin/plugin/src/org/jetbrains/kotlin/formver/plugin/compiler/UniquenessDeclarationChecker.kt
Outdated
Show resolved
Hide resolved
...ugin/plugin/src/org/jetbrains/kotlin/formver/plugin/compiler/UniquenessDeclarationChecker.kt
Outdated
Show resolved
Hide resolved
...ugin/plugin/src/org/jetbrains/kotlin/formver/plugin/compiler/UniquenessDeclarationChecker.kt
Outdated
Show resolved
Hide resolved
formver.compiler-plugin/testData/diagnostics/uniqueness_checker/assign_local.kt
Show resolved
Hide resolved
formver.compiler-plugin/testData/diagnostics/uniqueness_checker/assign_local.kt
Show resolved
Hide resolved
...ler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessGraphAnalyzer.kt
Outdated
Show resolved
Hide resolved
...iler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessGraphChecker.kt
Outdated
Show resolved
Hide resolved
...iler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessGraphChecker.kt
Outdated
Show resolved
Hide resolved
...ver.compiler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessTrie.kt
Outdated
Show resolved
Hide resolved
...ver.compiler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessType.kt
Show resolved
Hide resolved
…r/plugin/compiler/UniquenessDeclarationChecker.kt Co-authored-by: Komi Golov <komi.golov@jetbrains.com>
...ugin/plugin/src/org/jetbrains/kotlin/formver/plugin/compiler/UniquenessDeclarationChecker.kt
Outdated
Show resolved
Hide resolved
…rmver/uniqueness/UniquenessGraphChecker.kt Co-authored-by: Komi Golov <komi.golov@jetbrains.com>
…is' into marco/uniqueness-dataflow-analysis
…rmver/uniqueness/UniquenessGraphAnalyzer.kt Co-authored-by: Komi Golov <komi.golov@jetbrains.com>
|
It will be nice to cleanup/squash commits before merging. There are too many of them now. |
ligee
left a comment
There was a problem hiding this comment.
Squash some commits please, we don't need 70 commits in the history.
...r.compiler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/DataFlowAnalyzer.kt
Show resolved
Hide resolved
...ugin/plugin/src/org/jetbrains/kotlin/formver/plugin/compiler/UniquenessDeclarationChecker.kt
Outdated
Show resolved
Hide resolved
jesyspa
left a comment
There was a problem hiding this comment.
This looks good, I think we should get it merged. Not sure whether fixing the semantics of the tests is best done now or after the merge, since I think the structure is going to stay the same
formver.compiler-plugin/testData/diagnostics/uniqueness_checker/assign_local.kt
Show resolved
Hide resolved
...iler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessGraphChecker.kt
Show resolved
Hide resolved
...ver.compiler-plugin/uniqueness/src/org/jetbrains/kotlin/formver/uniqueness/UniquenessType.kt
Outdated
Show resolved
Hide resolved
…is' into marco/uniqueness-dataflow-analysis
|
I will cleanup the commit history once I finish with the requested changes. Should I keep some intermediate commits or is squash-merging preferable? |
I would prefer some logical grouping when needed. The PR of this size will probably benefit from some division. But reasonable amount, not 70. A single one is fine to me too. |
This PR introduces a DFA-based uniqueness checking system providing a more precise and flexible implementation compared to the original recursive traversal approach of the FIR AST. The new implementation operates on the Control Flow Graph (CFG) provided by the Kotlin compiler's
kotlin.fir.resolve.dfa.cfgmodule.Architecture
The implementation consists of two main components:
Analyzer
DataFlowAnalyzer.kt: Is a generic, worklist-based dataflow analysis facility for CFG nodes providing:FlowFactsresult object containingflowBeforeandflowAfterinformation for each CFG node.UniquenessGraphAnalyzer.kt: Performs forward-flow analysis to infer uniqueness types at each CFG node. The analyzer:UniquenessTrie).Checker
UniquenessGraphChecker.kt: Validates uniqueness invariants using the analysis results, currently checking:Supporting Constructs
Uniqueness Types
The implementation uses an algebraic datatype
UniquenessTypeinstead of passing around separateUniquenessLevelandBorrowLevelenums:Moved: Represents the top type in the lattice (a consumed/invalidated value)Active(UniqueLevel, BorrowLevel): Represents active values with:UniqueLevel: EitherUniqueorShared.BorrowLevel: EitherConsumedorBorrowed.Paths
Paths represent logical units holding uniqueness information. Concretely they are a list of Fir symbols able to represent:
Path.ktprovides also the means to extract paths from arbitrary expressions with:receiverPath: Extracting paths from atomic receiver expressions.valuePaths: Extracting potentially multiple paths from value expressions.Default Uniqueness Resolver
UniquenessResolver.ktfetches default uniqueness types for symbols (properties, functions, and function parameters) based on their annotations.Uniqueness Typing Environment
The uniqueness typing environment is represented by
UniquenessTrie.kt: A trie-based data structure to store uniqueness information about paths. Each trie level corresponds to one path component. If the accessed path doesn't have an associated uniqueness type, the trie lazily computes the default value for that path viaUniquenessResolver. The trie also supports ajoinoperation to join an instance in-place with another; we use this operation to merge multipleUniquenessTries coming from separate in-flows using dataflow analysis.Current Limitations
The implementation currently assumes a three-address code form where expressions are atomic (locals or fields). This means that the following features are not supported:
Nested Scopes and Shadowing
Variables shadowing other variables in inner scopes are treated as the same variable. Example:
This may lead to loss of uniqueness information in outer scopes.
Non-Atomic Expressions
Expressions containing paths must be atomic. Complex expressions like conditional returns are not yet supported:
Each function argument must be a single path; passing multiple possible paths via complex expressions is currently unsupported.
Testing
This implementation includes a set of reorganized test cases covering: