-
-
Notifications
You must be signed in to change notification settings - Fork 668
[DO NOT MERGE] Implement PoC DFA engine for nullability and truthiness #21494
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
|
Thanks for your pull request and interest in making D better, @rikkimax! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#21494" |
c1f7ebf to
f85972d
Compare
|
Ugh |
a4354e5 to
b741ece
Compare
shouldn't a and b both be null at that point? |
Yes, it's a false positive and needs fixing on my end. |
dccf42e to
4bf8120
Compare
|
True positives:
Overall the interprocedural nullability analysis looks to be working. Although unit-threaded will need to be analysed (can't be bothered to do it atm). |
5c26677 to
699ec5e
Compare
d2ff64e to
fbc977e
Compare
|
Waiting on this PR to go green and the following PR's to be merged.
Will follow with final PR after this. |
| Is any symbol this scope is applied known to have a compile time only accessible context. | ||
| This is not the same as `skipCodegen` nor can it be used to contribute to this. | ||
| It is meant for analysis engines to be conservative in what functions they analyse, | ||
| to prevent perceived false positives for meta-programming heavy code. | ||
| */ | ||
| bool knownACompileTimeOnlyContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar?
Is any symbol this scope is applied [to?] known to have a compile time only accessible context.
bool knownACompileTimeOnlyContext; feels like it should be
knownCompileTimeOnlyContext or knownAtCompileTimeOnlyContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Singular is correct, it may not be exclusive to CT.
So something like knownToHaveANonExclusiveCompileTimeOnlyContext.
However this isn't the right way to solve it long term. The right solution is to have something like @__ctfe and tell people to annotate their code appropriately rather than letting the compiler ignore problems. I sent Walter and Atila an email requesting that they think about where they want to go here, but that doesn't appear to have gone anywhere.
The adding of to will happen on follow up PR.
| /// Information for data flow analysis per parameter | ||
| extern(D) struct ParameterDFAInfo | ||
| { | ||
| /// Parameter id: -1 this, -2 return, otherwise it is FuncDeclaration.parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FuncDeclaration.parameters index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, updated for next PR.
| "Sema1: Function ", | ||
| "Sema2: ", | ||
| "Sema3: ", | ||
| "DFA: ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data Flow Analysis. also feel free to add more events (if there are logical places to put them) if it makes producing statistics for how much time this consumes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is enough for the fast DFA.
Having only like 8 instances show up on 100k LOC codebase is pretty telling and they are 1-2ms each.
I'm sure it'll need adjusting for the error pass, and slow DFA, but we aren't there right now.
Throwing the experimental fast DFA engine at CI to see what happens, this PR is not going to be merged can be ignored.