Skip to content

Nullability Analysis #7520

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 35 commits into from
Closed

Nullability Analysis #7520

wants to merge 35 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 8, 2019 •
edited
Loading

A simple nullability analysis.

The analysis tracks all immutable vals and local immutable vars (under the condition that all assignments to such vars are in places we can analyze). The analysis works also for inlined code and is thus able to recognize that after an assert(x != null) x is indeed not null.

The main issue right now is that sjsJUnitTests had to be disabled, because it did not link anymore after the change in DottyPredef that changed assertFail to have Nothing result type. The change to assertFail is needed so that nullability info is correctly propagated for assert. This looks like a JS issue that should be fixed,

Another issue is that nullable.scala fails pickling and FromTasty tests because the nullability info cannot be reproduced after pickling. It looks like this is fixable by making not-null assertions manifest with special function calls in the code. That scheme should be part of a subsequent explicit nulls PR.

@odersky odersky force-pushed the add-flow-analysis branch 4 times, most recently from 04388a4 to 39da9ee Compare November 8, 2019 22:42
@odersky odersky changed the title Thread context through typedStats Nullability Analysis Nov 8, 2019
@odersky odersky marked this pull request as ready for review November 8, 2019 23:30
@odersky odersky requested a review from abeln November 8, 2019 23:31
@odersky
Copy link
Contributor Author

odersky commented Nov 8, 2019

test performance please

@dottybot
Copy link
Member

dottybot commented Nov 8, 2019

performance test scheduled: 1 job(s) in queue, 0 running.

@odersky odersky mentioned this pull request Nov 8, 2019
Closed
@dottybot
Copy link
Member

dottybot commented Nov 9, 2019

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/7520/ to see the changes.

Benchmarks is based on merging with master (3cb93f3)

@sjrd
Copy link
Member

sjrd commented Nov 9, 2019

I'm pretty sure the Scala.js issue is the one being addressed in #7378

@odersky
Copy link
Contributor Author

odersky commented Nov 10, 2019

@sjrd @Lacaranian OK, then it would be good to get #7378 in first.

@odersky
Copy link
Contributor Author

odersky commented Nov 10, 2019

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/7520/ to see the changes.

Benchmarks is based on merging with master (3cb93f3)

@odersky
Copy link
Contributor Author

odersky commented Nov 10, 2019

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/7520/ to see the changes.

Benchmarks is based on merging with master (3cb93f3)

@odersky
Copy link
Contributor Author

odersky commented Nov 12, 2019

test performance please

@dottybot
Copy link
Member

performance test scheduled: 3 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/7520/ to see the changes.

Benchmarks is based on merging with master (4d3f315)

abeln
abeln approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@abeln abeln left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I only looked at Nullables.scala (busy this week finishing my thesis). I left a few comments, most of which are about adding additional documentation to methods.

Are you planning on merging this right away?

@sjrd
Copy link
Member

sjrd commented Nov 13, 2019

#7378 has been merged. You should be able to rebase to get rid of the Scala.js issue.

odersky added 12 commits November 13, 2019 12:52
@odersky
Track assignments for nullability
5cb0873
@odersky
Allow local variables to be tracked for nullability
6d5ed25
@odersky
Drop debug output
74d5b1e
@odersky
Constant fold null comparisons only under -Yexplicit-nulls
f1e2863
@odersky
Handle while expressions correctly for nullability
ed00d96
@odersky
Don't widen T | Null in widenUnion
926b08e
@odersky
Introduce NotNull type
947c49e 
NotNull is a common supertype of AnyRef and AnyVal. It will be interpreted specially
in type comparisons.
@odersky
Simplify in widenRHS
62b756b 
When computing the type for local type inferennce, use a simplify
transformation.
@odersky
Special treatment of NotNull in TypeComparer
e6746e5 
Makes use of the fact that `Null & NotNull = Nothing`. This fact is
used in both type comparisons and glb operations.
@odersky
Temporary hack to support testing
f32f22a 
For now, we treat _every_ type spelled "Null" as the Null type. That way
we can introduce a user-level Null that sits next to AnyVal and AnyRef, like
the real Null will once explicit nulls are in. This hack should be reverted
once we have the changes for Null implemented.
@odersky
Test case
e79dc49 
This test implements a notNull operation and demonstrates that it works
as required.

Also: Fix repltest
@odersky
Blacklist notNull as from Tasty test
e56743b 
There seems to be a missing simplification when compiling from Tasty.
Here is the error message:

```
*** error while checking out/posTestFromTasty/pos/notNull/Test.class after phase readTasty ***
java.lang.AssertionError: assertion failed: Types differ
Original type : (x: Int): Int
After checking: (x: (Int | Null) & NotNull): (Int | Null) & NotNull
Original tree : identity[(Int | Null) & NotNull]
After checking: identity[(Int | Null) & NotNull]
```
@odersky
Copy link
Contributor Author

odersky commented Nov 13, 2019

@sjrd Reverted. Great that the fix was in so quickly!

@odersky
Address review suggestions
b32e30a 
Better document Nullables.scala. Also, rename containsRef to impliesNotNull.
abeln
abeln reviewed Nov 13, 2019
View reviewed changes
compiler/src/dotty/tools/dotc/typer/Nullables.scala Outdated
* Therefore, variables with unreachable assignments can be assumed to be not-null
* only if their type asserts it.
*
* Note: we the local variables through their offset and not through their name
Copy link
Contributor

Choose a reason for hiding this comment

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

"we the local" -> "we track the local"

@odersky odersky force-pushed the add-flow-analysis branch 4 times, most recently from d7ce81a to 5697fe5 Compare November 14, 2019 09:47
@odersky
Add notNull member to Any
1726660 
Add a method $nn to Any that has type

    def $nn: this.type & NotNull

The method is marked as stable & realizable, which means it can appear in paths.
Special handling is required in asSeenFrom, to reflect the fact that $nn are not
real selections, i.e. the selected item has the same owner as the one in the prefix.

N.B. I tried to make $nn a field instead, but this caused AbstractMethod errors
for reasons I cannot quite track down.
@sjrd
Copy link
Member

sjrd commented Nov 14, 2019

@odersky Could you explain in a few sentences how the non-nullability analysis interacts with the NotNull class in this PR? I tried simply removing the definition of NotNull in master...dotty-staging:add-flow-analysis-without-notnull, without touching the nullability analysis per se, but the nullable.scala does not pass anymore. I don't understand in what capacity the only existence of NotNull allows that test to pass, given that the non-nullability analysis does not seem to reference NotNull anywhere.

@sjrd sjrd mentioned this pull request Nov 14, 2019
Merged
@sjrd
Copy link
Member

sjrd commented Nov 14, 2019

OK after more investigation, I realized that my mistake had something to do with the constant folder, not with the nullability analysis nor NotNull. PR at #7556.

@odersky
Copy link
Contributor Author

odersky commented Nov 15, 2019

Closed in favor of #7556

@odersky odersky closed this Nov 15, 2019
@Blaisorblade Blaisorblade deleted the add-flow-analysis branch November 24, 2019 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abeln abeln abeln approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@odersky @dottybot @sjrd @abeln