Summary:
Previous diff introduced convert_complex_exp_to_pvar that does not
modify typestate.
Luckily enough, experiement shows that we don't need it in this place!
Reviewed By: artempyanykh
Differential Revision: D20284942
fbshipit-source-id: ea1471681
Summary:
This function is converting exp to pvar AND sometimes modifies a
typestate (even when ~is_assignment is false, which is something one
would not expect!).
We aim to reduce cases when it is not needed.
This diff splits the function into two: good and evil, and uses good
when it is a no-op.
Reviewed By: ngorogiannis
Differential Revision: D20284301
fbshipit-source-id: 0e5d65a65
Summary:
Extract `Typ.Name.Java.Split` into a standalone module.
- This module is really only used in `Procname` so no need to be in `Typ`.
- Remove some pointless conversions to strings and back.
- Reduce the interface of `Split` to the minimum ahead of possible removal in favour of normal types.
Reviewed By: mityal
Differential Revision: D20322887
fbshipit-source-id: 7963757cb
Summary:
These are important enough operations to be tracked in logs.
This diff also tweaks logs for the main loop processing instuctions, so
that we log instr before processing it, not the reverse
Reviewed By: artempyanykh
Differential Revision: D20282443
fbshipit-source-id: 40b8b6627
Summary:
These were not used (and were actually activated byt the same config
param). They both are in experimental stage that never reached maturity.
Since the team does not have immediate plans to work on ObjC nullability
checker; and since "eradicate" (now known as nullsafe) is the main
solution for Java, removing it is sensible.
Reviewed By: jvillard
Differential Revision: D20279866
fbshipit-source-id: 79e64992b
Summary:
Both TypeOrigin.Undef and TypeOrigin.OptimisticFallback are bad and
should be killed.
Let's start with Undef as the biggest offender, then try to gradually
reduce usage of the second one.
It is super unclear what does Undef even mean, and actually the code that
tries to use it (in a way that I can not fully comprehend) occurred to be "almost" no-op.
"Almost" means that the only place that is affected is
CONDITION_REDUNDANT checks: we have few extra (FP) warnings of his type.
Mostly those correspond to comparing with null result of array member
access: `myArray[i] == null` is marked as redundant. And I even don't
know how come it was not showing up before: we do assume all calls to
array are optimistically non-null (see TypeOrigin.ArrayAccess).
Anyways, we give up on this one: condition redundant is "broken" anyway
(has too many FP to be surfaced to the user); and when/if we want to fix
it, we can easily support array access idiomatically.
Reviewed By: jvillard
Differential Revision: D20248988
fbshipit-source-id: b20f61fd0
Summary:
`TypeCheck.typecheck_node` is one of central parts in nullsafe
typechecking.
Lets make the code clearer:
1. Clear contract of the method - return a named record instead of a
tuple of lists.
2. Comments.
3. Making code functional, use List.fold and aggregate all needed data
there instead of storing information in references
4. Helper functions.
Reviewed By: skcho
Differential Revision: D20246273
fbshipit-source-id: 75f56497e
Summary:
1. It is convenient to stick with the policy "ERROR if and only if it is
enforced". Among other, it makes CI integration much easier to implement
(enforcemend, UI and messaging is decided based on severity).
2. Since Nullsafe annotation is an idiomatic way to indicate classes
with enforced nullability checking, we want it to be the only way to
enforce issues.
3. This means we decrease the priority of GraphQL violation issues.
(In practice they were not enforced so we have plenty of violations in
codebase to reflect reality). The proper way dealing with GraphQL will
be detecting such issues as a special issue type and prioritizing fixing
and Nullsafe-ifying corresponding classes.
4. Among other, we downgrade severity of field overannotated to advice
to keep it consistent with condition redundant.
Reviewed By: artempyanykh
Differential Revision: D20141420
fbshipit-source-id: e2f12835a
Summary:
For Mode.Local this is kind of obvious decision.
But this diff does the same for strict mode as well.
See comment in [ExplicitNonnullThirdParty] for the detailed explanation.
Reviewed By: artempyanykh
Differential Revision: D20140056
fbshipit-source-id: 13c66df81
Summary:
In the previos diff we restructured error rendering utils for
TypeOrigin.MethodCall.
In this diff we do the same with TypeOrigin field: lets make the code
consistent.
We also clearly distinct third party from all other possible cases in
this branch.
This changes messaging and reported errors for strict modes (see test cases), and I believe this is a net improvement.
Reviewed By: artempyanykh
Differential Revision: D20139741
fbshipit-source-id: 84f502553
Summary:
Since artempyanykh introduced proper type for third party methods, we don't need
to write a sketchy heuristic in this place.
This will simplify shipping a feature in the follow up diff (otherwise
it would break here).
Reviewed By: artempyanykh
Differential Revision: D20139460
fbshipit-source-id: 00144dc48
Summary:
Now when typechecking a class `A` marked with `Nullsafe(LOCAL)`,
classes from trusted list are properly recognized and nullability of
method params and return value are refined to `LocallyCheckedNonnull`
in a context of class `A`.
NOTE: refininng nullability when **accessing fields** on trusted classes
is **not implemented yet**, because the whole business of handling fields
in nullsafe is somewhat convoluted. This should not be a huge issue
though, since in Java fields are commonly accessed via getters any
way.
Reviewed By: mityal
Differential Revision: D20056158
fbshipit-source-id: 496433d90
Summary:
This will help making error reporting more actionable.
Often methods that are nullable in general (like View.findViewById) are used as not-nullable due to app-invariants. In such cases suggesting a non-nullable alternative that does an assertion under the hood makes the error report more actionable and provides necessary guidance with respect to coding best practices
Follow up will include adding more methods to models.
If this goes well, we might support it in user-defined area (nullability
repository)
Reviewed By: artempyanykh
Differential Revision: D20001416
fbshipit-source-id: 46f03467c
Summary:
Introduction of `ThirdPartyNonnull` nullability broke nullability
refinement heuristic for enums. This diff fixes it and also adds tests
so that we hopefully avoid such issues in future.
Reviewed By: mityal
Differential Revision: D19975810
fbshipit-source-id: f9245f305
Summary:
We need to be able to differentiate `UncheckedNonnull`s in internal vs
third-party code. Previously, those were under one `UncheckedNonnull`
nullability which led to hacks for optmistic third-party parameter
checks in `eradicateChecks.ml` and lack of third-party enforcement in
`Nullsafe(LOCAL, trust=all)` mode (i.e. we want to trust internal
unchecked code, but don't want to trust unvetted third-party).
Now such values are properly modelled and can be accounted for
regularly within rules.
Also, various whitelists are refactored using
`Nullability.is_considered_nonnull ~nullsafe_mode nullability`.
`ErrorRenderingUtils` became a tad more convoluted, but oh well, one
step at a time.
Reviewed By: mityal
Differential Revision: D19977086
fbshipit-source-id: 8337a47b9
Summary:
Add support for nullsafe mode with `trust=all` and `trust=none` a case
with a specific trust list is not supported yet and needs to be
implemented separately.
Tests introduce one unexpected
`ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION` issue which
complains about `this` having incorrect nullability; it is a bug and
needs to be fixed separately.
Reviewed By: mityal
Differential Revision: D19662708
fbshipit-source-id: 3bc1e3952
Summary: In all other cases we have period at the end, which is inconsistent.
Reviewed By: artempyanykh
Differential Revision: D20001065
fbshipit-source-id: 85ec6d751
Summary:
This helps debug nullsafe. Before, we would only print the initial and
last state of a given node but now we can see all the intermediate steps
too.
Example before:
```
before:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
after:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
```
After:
```
before:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
instr: n$0=*&this:Toto* [line 10]
new state:
n$0 -> [this] [StrictNonnull] Toto*
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
...
instr: EXIT_SCOPE(n$0,n$1,this); [line 10]
new state:
&s -> [Param s ] [UncheckedNonnull] java.lang.String*
&this -> [this] [StrictNonnull] Toto*
```
Reviewed By: mityal
Differential Revision: D19973278
fbshipit-source-id: bcea33f96
Summary:
Use a record of package, class name to store (qualified) Java class names. This saves the round trip of concatenating then splitting again, etc, as well as saves some memory in the type environment as now the package paths can be shared across classes of the same package (about 10% in tests).
Also remove some unfortunate APIs.
Reviewed By: jvillard
Differential Revision: D19969325
fbshipit-source-id: f7b7f5a55
Summary:
Some annotation processors / transormers might generate artificial fields. These
are not expicitly written by code writer, hence non actionable.
We distinct this fields heuristically: if they start with "$", they are
surely not user-written.
Reviewed By: ezgicicek
Differential Revision: D19947988
fbshipit-source-id: a0a15fc23
Summary:
The big one:
- stop using polymorphic `<>`, `<`, `>`, ..
- add `<>` to `PolyVariantEqual` escape hatch now that `<>` is as taboo as `=`
- Interestingly, there were a lot of uses of `Z.(x < y)`, which although
they seem to use `Z.lt` actually used polymorphic comparison. The actual
comparison infix operators of `Z` are cleverly hidden in `Z.Compare`
instead, which makes them impractical to use...
Reviewed By: jberdine
Differential Revision: D19861584
fbshipit-source-id: 5dce08ad9
Summary:
This was introduced in D19770219, and skcho caught it, but for some
reason I seemed to have a blind spot and confused the semantics of `instanceof`.
Reviewed By: skcho
Differential Revision: D19813031
fbshipit-source-id: d939b981b
Summary:
We already warn about lack of nullable annotations in `equals()`, and even have a specialized error message for that.
But lack of an annotation is not as severe as direct dereference: the
latter is a plain bug which is also a time bomb: it will lead to an NPE not immediately.
This is widespread enough to be reported separately.
Reviewed By: dulmarod
Differential Revision: D19719598
fbshipit-source-id: a535d43ea
Summary:
Since we fixed a bug in implementation of FalseOnNull (see stack below),
we can finally ship this change.
Side note: this change is essential for the follow up diff (which adds extra check
for user-defined implementations of equal()), without it the follow
up change would introduce a lot of false positives.
Reviewed By: ngorogiannis
Differential Revision: D19771057
fbshipit-source-id: 7d7cf1ef7
Summary:
If we managed to whitelist a function as TrueOnNull, we should teach
nullsafe the nullability of its arguments, otherwise it will ask not to
pass null here.
This fixes a silly FP warning, see the test.
Reviewed By: dulmarod
Differential Revision: D19770341
fbshipit-source-id: 0f861fae1
Summary:
Yay, the previous refactoring finally makes it possible to do some actual
changes to the code in `TypeCheck.ml`!
Changes in this diff:
1. Fixes the bug: TrueOnNull and FalseOnNull were working only for
static methods. Surpsingly nobody noticed that. It is because the first
argument for non-static method was `this`.
2. Behavior change: TrueOnNull/FalseOnNull were not working correctly
where there are several argumens. See the task attached for the example
of the legit usecase. Now the behavior is the following: if there are
several Nullable arguments infer nullability for all of them.
Reviewed By: skcho
Differential Revision: D19770219
fbshipit-source-id: 7dffe42cd
Summary:
This diff proceeds clean up the mess in TypeState.ml
This refactoring unblocks the change in the logic for true on null and
allows to fix a bug, see follow up diffs.
The current code is trying to do two things at once: processing boolean
results (which normally need manipulations with the argument of a
function + additional logic for containsKey), and comparisons witn null
(which requires manipulation with return value in typestate plus check
for redundancy).
All this was done in sort of generic fashion, which, among other, lead
to weird situations for edge cases, e.g. in processing containsKey we used to override nullability
already inferred as a non-null twice with losing information about
original nullability; which made logs weird).
Reviewed By: ngorogiannis
Differential Revision: D19768795
fbshipit-source-id: c928e2cff
Summary:
This diff is part of cleaning up of Typestate.ml mess to make it
somewhat maintainable.
This method is always called with `default` equivalent to (exp,
typestate); also there is no semantical need to return typestate because it never
gets changed.
Reviewed By: ngorogiannis
Differential Revision: D19767366
fbshipit-source-id: 173dcbbca
Summary:
This refactoring is made possible by previous stack, which ensured we
don't do two completely different things in one code anymore (processing
results of functions returning booleans and objects in generic fashion).
Follow up diffs will clean up the code for Prune(a != zero) case.
Reviewed By: dulmarod
Differential Revision: D19745050
fbshipit-source-id: 61d3d02ad
Summary:
This refactoring unblocks the changes in follow up diffs (plus fixes a
bug).
So what was happening?
Each comparison with null leads to CFG being splitted into two branches, one branch
is PRUNE(a == null) and another is PRUNE(a != null).
PRUNE(a != null) is where most of logic happens, it is the place where
we infer non-null nullability for a, and this is a natural place to
leave a check for redundancy.
Before this diff we effectively checked the same thing twice, and used
`true_branch` (only one of 2 instruction will have it set to true) as a symmetry breaker.
This diff removes the `true_branch` checks, but leaves only one call out
of two, hence breaking symmetry in a different way.
## Bug fix
The code around the removed check was (crazily) doing two things at
once: it processed results of (returning booleans!)
TrueOnNull-annotated functions AND
results of (returning Objects!) other functions, using the fact that all
of them are encoded as zero literals (sic!).
Not surprisingly that lead to a bug where we accidentally call the check
for non intended places (arguments of trueOnNull functions), which lead
to really weird FP.
This diff fixes it.
Reviewed By: dulmarod
Differential Revision: D19744604
fbshipit-source-id: fe4e65a8f
Summary:
These two methods are called in processing prune instructions, when
instruction is Prune(expr == null) and Prune(expr != null), to correctly
infer nullability in corresponding branches.
Typechecking underlying expr makes little sense for two reasons:
1. In practice, expr it is as simple as a temporary SIL variable
2. If the idea is defensively typecheck everything for case when SIL
produces crazy expressions, well, that is not going to work: the code
around ignores many other forms of expressions, e.g. everything where
expr = <something not equal to null literal>. So this is inconsistent.
This will simplify further cleanup, see follow up diffs
Reviewed By: ngorogiannis
Differential Revision: D19743826
fbshipit-source-id: 319a80ee7
Summary:
The whole TypeCheck.ml is exceptionally hard to read and maintain, lets
clean up it a bit.
Reviewed By: ngorogiannis
Differential Revision: D19743632
fbshipit-source-id: c24c21a85
Summary:
Refactor all occurences of `is_strict_mode` to use `NullsafeMode`
instead. This will allow introducing _local_ typechecking modes for
nullsafe in the follow up patches.
Reviewed By: ezgicicek
Differential Revision: D19639883
fbshipit-source-id: bdf535b66
Summary:
Rename DeclaredNonnull -> UncheckedNonnull, Nonnull -> StrictNonnull.
This is a preparatory step to introduce one more nullability option,
specifically CheckedNonnull to support local nullsafe mode.
Reviewed By: ngorogiannis, mityal
Differential Revision: D19619289
fbshipit-source-id: 12a3ff814
Summary:
This will help adoption of non-transitive strictification modes.
The main use-case we are aiming with this function is:
- During strictification, a method is made nullable, but it is not feasible to fix all
callers straight away. Suppressing nullsafe with an appropriate task or
comment will make it easier to unblock the strictification and move
forward.
Reviewed By: artempyanykh
Differential Revision: D19599098
fbshipit-source-id: 3769bbd3d
Summary:
1. I can not imagine a useful usecase for strict node, lets not
complicate the code.
2. Add docs.
Reviewed By: artempyanykh
Differential Revision: D19599091
fbshipit-source-id: 1d37a717f
Summary:
In practice, condition redundant is extremely noisy and low-signal
warning (hence it is turned off by default).
This diff does minor tweaks, without the intention to change anything
substantially:
1/ Change severity to advice
2/ Change "is" to "might be"
3/ Describe the reason in case the origin comes from a method.
The short term motivation is to use 3/ for specific use-case: running nullsafe on codebase and
identify most suspicious functions (that are not annotated by often
compared with null).
Reviewed By: artempyanykh
Differential Revision: D19553571
fbshipit-source-id: 2b43ea0af
Summary:
This is/can be useful for future changes that make the reporting more
precise based on the exact origin.
See the follow up diff as an example.
Reviewed By: artempyanykh
Differential Revision: D19553567
fbshipit-source-id: c2b2c28a1
Summary:
This is a common enough case to make error message specific.
Also let's ensure it's modelled.
Reviewed By: artempyanykh
Differential Revision: D19431899
fbshipit-source-id: f34459cb3
Summary:
The previous diff changes the message for params case, this one handles
return.
Reviewed By: artempyanykh
Differential Revision: D19430706
fbshipit-source-id: f897f0e56
Summary:
As suggested by Ilya, the current message can be improved in a way that
it can contain more clear action. I also added artempyanykh's explanation at the
end of message to provide an additional justification from common sense
perspective.
But most importantly, the previous message was missing a space which is
eye bleeding, how come haven't I noticed this before, I can't stand it
OMG.
Reviewed By: artempyanykh
Differential Revision: D19430271
fbshipit-source-id: dd31f7adb
Summary:
Now we can either disable it, or enable it only.
For integrations that disable it, this will allow to enable it for
NullsafeStrict classes without enabled it fully
Reviewed By: artempyanykh
Differential Revision: D19409131
fbshipit-source-id: a2b1fe650
Summary: This diff implements this for Field Not Initialized check
Reviewed By: artempyanykh
Differential Revision: D19393989
fbshipit-source-id: cf60e8d53
Summary:
This diff does it for nullable dereference and assignment violations
rules which happen under NullsafeStrict case.
Follow up are to make the same for inheritance and field initializer
violations.
Possible follow up includes making error message more specific and
articulare this this is a nullsafe strict mode.
Reviewed By: artempyanykh
Differential Revision: D19392916
fbshipit-source-id: 2554ac7a7
Summary:
The one with Object as the second param is in `com.facebook.common.internal`.
The one with String as the second param is in `com.facebook.common.preconditions`.
(Don't ask :) )
Reviewed By: artempyanykh
Differential Revision: D19087334
fbshipit-source-id: ba02c9101
Summary:
According to Java semantics, they are always non-null.
Internally they are represented as static fields, so they have
DeclaredNonnull nullability, which means NullsafeStrict mode would
refuse to use them without strictification.
Lets teach nullsafe that these guys are non-nullables.
See also FN in test case.
Reviewed By: ngorogiannis
Differential Revision: D19024547
fbshipit-source-id: 8c120fa50
Summary:
- Remove `to_flat_string` as there is `get_field_name` that unambiguously does the same thing.
- Make `pp` print only the field in all languages.
- Fix `to_full_string` so that it has unified behaviour across java/clang and so that it doesn't print `class Foo.x`, but rather `Foo.x`.
Reviewed By: ezgicicek
Differential Revision: D18963033
fbshipit-source-id: e2c803c7d
Summary:
Remove Clang and Java submodules of Typ.Fieldname. They are unnecessary and they reflect a fake dichotomy: there is only one fieldname type. To distinguish between fields of Java classes and other C constructs, there is a helper function provided, but the idea is simple: obtain the class type the field belongs to, and check if it's a Java class.
This diff still preserves behaviour, but removes as many functions as possible from the interface, to leave a small surface.
Reviewed By: mityal
Differential Revision: D18962423
fbshipit-source-id: ffe6933ee
Summary:
For now lets support the simplest form: "//"-style comments.
Having comments is useful:
1. It serves as documentation for API.
2. It serves as justification of why such and such method or param is
nullable or not (so the future readers can understand the reasoning of
the person who added the signature).
Reviewed By: artempyanykh
Differential Revision: D18762289
fbshipit-source-id: 90c515ea8
Summary:
This diff enables parsing and auto-formatting documentation
comments (aka docstrings).
I have looked at this entire diff and manually made some changes to
improve the formatting. In some cases it looked like it would take too
much time, or benefit from someone more familiar with the code doing
it, and I instead disabled auto-formatting docstrings in those files.
Also, there are some source files where the docstrings are invalid,
and some where the structure detected by the parser appears not to
match what was intended. Auto-formatting has been disabled for these
files.
Reviewed By: ezgicicek
Differential Revision: D18755888
fbshipit-source-id: 68d72465d
Summary:
We already have a number of `[pkg].Preconditions.checkNotNull`
modelled, but the one from `androidx` is missing yet widely used.
Reviewed By: mityal
Differential Revision: D18748550
fbshipit-source-id: 83d3317ae
Summary:
This will make our analysis more precise. E.g. in the future we can classify
"condition redundand" warnings by origin, and warn only for
InferredNonnull, or autogenerate fixes based on the origin.
Error messaging does not currently take into account origin for nonnull
types, but this can be changed in the future
Reviewed By: ngorogiannis
Differential Revision: D18685304
fbshipit-source-id: 6a1f263f5
Summary:
This will allow us tune nullsafe behavior in a more fine-grained way.
See e.g. a follow up diff when we report errors more clearly.
Reviewed By: ngorogiannis
Differential Revision: D18683747
fbshipit-source-id: 7b5c42a03
Summary:
We were wrongly not supressing third-party calls in
case third party repo is provided: there was a bug in the function is_modelled_externally.
because of this we accidentally stopped supressing third party and
started to advertise it in non-strict mode.
we do plan doing that, but little bit further ahead :)
So let's add a param guiding this.
Reviewed By: artempyanykh
Differential Revision: D18658271
fbshipit-source-id: 703f2675b
Summary:
We will migrate to the new representation in the future, but for now
let's keep both active.
Reviewed By: artempyanykh
Differential Revision: D18657955
fbshipit-source-id: 9deb03f1f
Summary:
Now we point to the root cause of the problem, and also provide
actionable way to solve the issue
Reviewed By: artempyanykh
Differential Revision: D18575650
fbshipit-source-id: ba4884fe1
Summary:
Two goals:
1. Be less assertive when speaking about third party code (it might be
written with different conventions).
2. Point to third party signatures folder so the users know how to
proceed
Reviewed By: artempyanykh
Differential Revision: D18571514
fbshipit-source-id: 854d6e746
Summary:
1/ We now support messaging for third-party: show file name and line
number
2/ We did not show information about internal models in case of param
calls
3/ Small change: we don't specify "modelTables.ml" anymore: no need to
expose implementation details
Reviewed By: artempyanykh
Differential Revision: D18569790
fbshipit-source-id: 28586c8ff
Summary:
In the follow up diff we will use it to render more informative
error message when type error is relevant to the fact that the code is
third-party
Reviewed By: artempyanykh
Differential Revision: D18569783
fbshipit-source-id: 4f6dcc404
Summary:
Let the functions return the modified storage, not unit.
It will play nicely with the next diff, which will add more data to
storage which will be non-mutable.
Reviewed By: artempyanykh
Differential Revision: D18569768
fbshipit-source-id: ade5685be
Summary:
Whole bunch of changes aimed to make error messages more clear and
concise.
1/ Wording and language is unified. We make errors sound more like a
type system violations, rather than linter reccomendations.
Particularly, we refrain from saying things like "may be null" - this is
a linter-style statement that may provoke discussions (what if the
developer knows it can not be null in this particular case).
Instead, we refer to declared nullability and nullability of actual values. This way, it is more clear that this is not a heuristic, this is how rules of a type-system work.
2/ Additionally, we drop things like field class in places when the
context should be clear by who looks at the error. We expect the user
sees the code and the error caption. So e.g. we don't repeat the word "field"
twice.
3/ In cases when we are able to retrieve formal param name, we include it for
usability.
4/ For Field not initialized error, we refer to Initializer methods:
this is a non-obvious but important nullsafe feature.
Reviewed By: artempyanykh
Differential Revision: D18569762
fbshipit-source-id: 9221d7102
Summary:
It make the message bit less heavy, and also it is kind of obvious that
it is origin.
In follow up diffs we will change the text so it is hopefully even more
obvious.
Reviewed By: artempyanykh
Differential Revision: D18527695
fbshipit-source-id: a305d547b
Summary:
1. We don't want to teach the users to ignore noise origin because
sometimes we are going to render something useful for them.
2. It just looks not cool.
Reviewed By: artempyanykh
Differential Revision: D18527694
fbshipit-source-id: 0ea248122
Summary:
This diff also removes clumsy typeErr.origin_desc and improves interface
of TypeOrigin.
This will allow to render smarter error message, depending on the
context (see follow up diff).
Reviewed By: ngorogiannis
Differential Revision: D18527692
fbshipit-source-id: b7eb11db8
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
We used to use default for this, which is both buggy and hard to
diagnose. Now we explicitly document nullsafe behaviour in this case.
Reviewed By: artempyanykh
Differential Revision: D18481220
fbshipit-source-id: 0b0cf8b38
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
This method was the last escape hatch for InferredNullability to
misreport about its origin.
Now each time we change nullability we provide origin that is consistent
with the nullability itself.
Reviewed By: artempyanykh
Differential Revision: D18453567
fbshipit-source-id: 3d9b7fa2e
Summary:
This is one of key diff in this refactoring stack.
Previously, we used to set nullability independently of type origin,
which opened doors for inconsistent states, bugs, and overcomplicated
code.
Now we have just two method left:
1. `create` -> the main one, yay
2. `set_nonnull` -> will get rid of this in the next diff.
Reviewed By: artempyanykh
Differential Revision: D18451950
fbshipit-source-id: edc485709
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
In this diff, we merely rename the value to clearly reflect its currect
usage.
In follow up diffs we are going to make usages of this instance more
restricted and concentrate them in several places in the code.
Reviewed By: artempyanykh
Differential Revision: D18451290
fbshipit-source-id: cf3773364
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
1. `Formal` was bit cryptic name.
2. Splitting method param and this makes a lot of sense. It is almost an
implementation detail that hey happen to come from "param" in the
method's signature.
3. Apart from others, this diff fixes a minor bug - we used to treat
this as DeclaredNonnull, which (in future) means suppressing legit warnings
like condition redundant. This would be an issue if we were to start
showing "high confidence" condition redundant warnings.
Reviewed By: ngorogiannis
Differential Revision: D18451294
fbshipit-source-id: acc295e3f
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
1. Make Field origin a record: consistent with Proc; more readable; will
add more elements in follow up diffs.
2. `Proc` is bit ambiguous: it is unclear if this is a method call result or param.
Let's make it clear that it is the result of a method call.
Reviewed By: ngorogiannis
Differential Revision: D18450188
fbshipit-source-id: c5abae3ad
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
Using `New` was definitely not a good idea here, we need a specific
data type value here.
Reviewed By: artempyanykh
Differential Revision: D18450174
fbshipit-source-id: 7ce7cc7e8
Summary:
This diff is a part of work teaching Nullsafe to explain decisions it's
making.
Introducing more specific origins will allow us to automatically infer
nullability based on type origin (in follow up diffs).
Reviewed By: ngorogiannis
Differential Revision: D18450165
fbshipit-source-id: 9dc0d25c0
Summary:
`typecheck_instr` is a function that essentially does most of nullsafe typechecking
work: it pattern matches the instruction and handles most of business logic (e.g. knowledge about builtins, rules of PropagatesNullable, flow-sensitive rule for condition branches etc etc etc.
This is a gigantic function consisting of huge amount of nested
function. No way it can fit in someone's head and it is super hard to
read. It also captures many things like node and node'.
This diff does fairly shallow work:
1. Moves most of nested subfunctions into top-level functions. It also
means that what was previously hiddenly captured is now made an explicit
dependency.
2. Some (minority) of them are renamed to better indicate the context.
3. Some (minority) of params are renamed to clear indicate the
difference.
4. Some params are made named params, especially booleans and params
that have the same types so could be confused.
5. In some placed, added bit more clear comments.
This diff DOES NOT do:
1. Tries to fully resolve confusion between cryptic names. In couple of places, instead of node'
and node we use node and original_node, which is barely better. But we
still have ugly things like typestate and typestate1 leaving together.
Baby steps!
2. Tries to create better or consistent names in functions. I did it for
most obvious cases, that's it.
3. Order or params is arbitrary. I just used the order that was easy to
come up with, no consistency here.
Reviewed By: artempyanykh
Differential Revision: D18425965
fbshipit-source-id: 7a775f08d
Summary:
We still have few remaining places which we have a logic not encoded in
as a Nullability type. Ignoring third party param calls is one of them.
We don't want this behavior for strict mode.
Reviewed By: artempyanykh
Differential Revision: D18396639
fbshipit-source-id: dbfedc769
Summary:
This is a dirty hack to make nullsafe behave consistently for internal
and external models.
Medium term this code needs to be rewritten in a better way, so that we
pass all information in annotated signature (either in form of Inferred
nullability, or type origin, which tracks third party calls).
As a follow up, we will make reporting more clear.
Reviewed By: artempyanykh
Differential Revision: D18372660
fbshipit-source-id: 12c2449e1
Summary:
Ability to accept relative path is convenient for testing and local
debugging.
Ability to accept absolute paths is needed for buck integration - buck
knows absolute path (to properly invalidate cache when the content of
the folder is changed) and passes it to infer.
Reviewed By: artempyanykh
Differential Revision: D18370407
fbshipit-source-id: be7f12ae1
Summary: Follow ups will include error messaging that makes the choice clear
Reviewed By: artempyanykh
Differential Revision: D18347664
fbshipit-source-id: b6f005726
Summary:
In this diff, we just load the info from the storage. Next diff will be
actually using this information to infer nullability.
`ThirdPartyAnnotationGlobalRepo.get_repo` will be used in the next diff,
hence #skipdeadcode
Reviewed By: artempyanykh
Differential Revision: D18347647
fbshipit-source-id: 82a9223c6
Summary:
In next diffs, we will:
1/ teach nullsafe to read nullability information from the 3rd party
annotation folder
2/ use this storage in addition to our hard-coded
models to respect nullability during type-checking
Reviewed By: artempyanykh
Differential Revision: D18247538
fbshipit-source-id: ee45bc80e
Summary:
More rigid format for 3rd party annotation repo
To simplify external processing by tools, lets not accept any divergence from a
format, which is:
<fully qualified class name>#<method_name>(<params>)[ Nullable]
<params> = <list of param separated by ", ">
<param> = [Nullable ]<fully qualified type>
Particularly:
- Dont accept spaces in arbitrary places
- Require a single space after each comma separating params
- Require a single space between closing parethesis and nullable
annotation, if present
- Require at least one '.' in a class name (to prevent errors when
somebody did not specify a package).
This diff also makes error messaging more user-friendly, because now it
is easy to do a minor mistake like forget a space after comma.
Reviewed By: asm89
Differential Revision: D18297382
fbshipit-source-id: 91aab6823
Summary:
This is a helper module for reading info from a 3rd party nullability repository.
Next diffs are going to use it for reading nullability repository from
disk.
Reviewed By: artempyanykh
Differential Revision: D18225473
fbshipit-source-id: 06a2dc97e
Summary: The way `<=` is used in `AbstractDomain` prevents infix use and forces bracketing it everywhere. Replace with simple `leq`.
Reviewed By: jvillard
Differential Revision: D18201854
fbshipit-source-id: 8175224e4
Summary:
Primitive types are not annotated. Because of that, we used to implicitly derive
`DeclaredNonnull` type for them. This worked fine, but this leads to errors in Strict mode, which does
not believe DeclaredNonnull type.
Now lets offifically teach nullsafe that primitive types are
non-nullable.
Reviewed By: jvillard
Differential Revision: D18114623
fbshipit-source-id: 227217931
Summary:
The wrong function was used when we tried to see if the class is
annotated with NullsafeStrict. This made it work only for non-static
methods.
Now we use the proper way.
Reviewed By: ngorogiannis
Differential Revision: D18113848
fbshipit-source-id: 02b7555be
Mitya Lyubarskiy
Nikos Gorogiannis
Artem Pianykh
Jules Villard
Ezgi Çiçek
Josh Berdine