Summary:
Currently, the notion of a third party method signature (corresponding to
a single record in a .sig file) is scattered between unique_repr and
nullability.
Logically, ThirdPartyMethod.t should not "know" about unique_repr
because this is a detail needed only for searching for a method
definition in `.sig` storage.
This diff:
1. Introduces ThirdPartyMethod.t which exposes exactly information
stored in .sig file. We are going to use this abstraction more in follow
up diffs.
2. Moves functionality operating with `unique_repr` to the module
responsible for searching in the repository.
3. Deletes `ThirdPartyMethod.nullability` abstraction - we don't need it as we can just store ThirdPartyMethod.t where was previously `nullability` stored.
4. Introduces `to_canonical_string` method that prints back third party
method in a format needed for `.sig` file, and a test ensuring that
parsing works back and forth consistently. We are going to use this
method in follow up diffs as well.
Reviewed By: artempyanykh
Differential Revision: D22950706
fbshipit-source-id: a0e72ccdb
Summary: This diff refactors Java specific `PatternMatch` functions into its own module. When `PatternMatch.ml` was originally created, it was mainly for Java but now it also supports ObjC. Let's refactor it to reflect the Java/ObjC separation: move all functions that operate on Java procnames into Java submodule.
Reviewed By: jvillard
Differential Revision: D22816504
fbshipit-source-id: ff6b64b29
Summary:
This diff:
1. Adds general capability to model any field as nullable /
non-nullable.
2. Uses it for Boolean.TRUE and Boolean.FALSE
Reviewed By: artempyanykh
Differential Revision: D22794226
fbshipit-source-id: 95f586592
Summary:
These 2 methods are automatically supplied for all enums, with
predefined behavior and nullability: https://www.geeksforgeeks.org/enum-in-java/
(Note that they are not part of java.lang.Enum class).
This will allow using them in unvetted third part and under strict mode.
Reviewed By: artempyanykh
Differential Revision: D21501716
fbshipit-source-id: 104082d15
Summary:
Based on a shrewd observation by mityal, we can copy a little bit of
code from nullsafe and cut the dependency between biabduction and
nullsafe. The trick was to notice that biabduction doesn't use the full
power of the functions it was calling.
Reviewed By: mityal
Differential Revision: D21282656
fbshipit-source-id: 906847c26
Summary:
Lets move the logic dealing with non-java classes outside of this module
so we can modify it easier in the next diff.
Reviewed By: artempyanykh
Differential Revision: D21204822
fbshipit-source-id: 67b5937bc
Summary:
This diff is a step forward to the state when the list of type violations is
independent of the mode (and we use mode solely to decide re: whether to
report or not).
This fixes a case when we incorrectly defined possible promo mode (see
the test payload)
Reviewed By: artempyanykh
Differential Revision: D20948897
fbshipit-source-id: 616b96f96
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:
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:
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:
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:
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 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:
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:
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: Follow ups will include error messaging that makes the choice clear
Reviewed By: artempyanykh
Differential Revision: D18347664
fbshipit-source-id: b6f005726
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
Summary:
This is the first take on strict mode semantics.
The main invariant of strict mode is the following:
If the function passes `NullsafeStrict` check and its return value is
NOT annotated as Nullable, then the function does not indeed return
nulls, subject to unsoundness issues (which should either be fixed, or
should rarely happen in practice).
This invariant helps the caller in two ways:
1. Dangerous usages of non strict functions are visible, so the caller is enforced to check them (via assertions or conditions), or strictify them.
2. When the function is strict, the caller does not need to worry about
being defensive.
Biggest known issues so far:
1. Condition redundant and over-annotated warnings don't fully
respect strict mode, and this leads to stupid false positives. (There is
so much more stupid false positives in condition redundant anyway, so
not particularly a big deal for now).
2. Error reporting is not specific to mode. (E.g. we don't distinct real nullables and non-trusted non-nulls, which can be misleading). To be
improved as a follow up.
Reviewed By: artempyanykh
Differential Revision: D17978166
fbshipit-source-id: d6146ad71
Summary:
"Unannotated" is misleading and ambiguous concept, it can have different
meanings depending on agreements.
The current logic treats them as Nonnull, which is exactly what we want
to preserve.
(If we need to partially model some functions where we don't have
opinion on some of types in the signature, we can explicitly model
unknown nullability later on).
Note that I am not aiming for substantial refactoring of
modelsTables.ml; the scope of this diff is merely to clarify things.
Reviewed By: ngorogiannis
Differential Revision: D17449347
fbshipit-source-id: 43c798ce7
Summary:
This function is the main entry point for getting annotated signature
for nullsafe.
We will modify it and its callees in follow up diffs to migrate other
features of Annot.items to specialized types.
Reviewed By: ngorogiannis
Differential Revision: D17448082
fbshipit-source-id: be00b4737
Summary:
This abstraction was not always used consistently.
Its usage made more sense when it supported both present annotations and
optional annotation (which got removed in previous diff).
The rought semantic of that was "what is the inferred type for such and
such value (variable or expression) in typestate". So it is not really
_annotation_ in first place, it is more like "what we inferred about
nullability given annotations, known special cases, and current sybmolic
execition state".
Let's explicitly rename `map` to `is_nullable`. If/when we need to
enhance this further (and we likely will), we will do it accordingly.
Reviewed By: jvillard
Differential Revision: D17153434
fbshipit-source-id: 3c85b56df
Summary:
`Present` annotation was an experiment made many years ago that never
got into real usage. The idea was to annotate Optional<> types with
Present, which means that it is safe to call get().
We don't plan to support `Present` annotation for optional types in the
near future.
Support of `Present` annotation requires extra levels of abstraction
that make the changing the behavior and introducing new features harder.
A lot of checks for nullability are written in generic way so they also
check for presense.
Getting rid of that will allow us to simplify our
work for introducing new semantics for nullsafe.
Reviewed By: ngorogiannis
Differential Revision: D17153432
fbshipit-source-id: c5ea9bdf1
Summary: Moving all the files related to nullable type checking under the same directory. The goal is to merge everything into the same backend based on the AI framework and access expressions.
Reviewed By: ngorogiannis
Differential Revision: D13350880
fbshipit-source-id: 8ab3cf81b
Mitya Lyubarskiy
Ezgi Çiçek
Jules Villard
Artem Pianykh
Nikos Gorogiannis
Josh Berdine
Jeremy Dubreil