Remove Library.sentinel.
#4155
Remove Library.sentinel.
#4155
Conversation
| final PackageGraph packageGraph; | ||
| @override | ||
| final Library library; | ||
| final Library? library; |
There was a problem hiding this comment.
I left Library as nullable anywhere a sentinel value could potentially flow (as well as I could deduce that, which isn't much - using null makes things easier to reason about).
| if (index == -1) return packageOrder.length * 10; | ||
| if (packageName == 'Dart' && | ||
| !_dartCoreLibraries.contains(element.library.name)) { | ||
| !_dartCoreLibraries.contains(element.library!.name)) { |
There was a problem hiding this comment.
Most of the places where a ! was added was in this file, where an element.library.property would already have thrown if the value was a sentinel.
The rest are where calling constructors for classes that expect a non-null library value, ensuring that we only get "no library" where a sentinel was expected to be used.
| // Instead of using `allModelElements`, which includes deeper elements like | ||
| // methods on classes, gather up only the library's immediate members. | ||
| var libraryMembers = [ | ||
| var libraryMembers = <ModelElement>[ |
There was a problem hiding this comment.
(Explicitly typed to avoid inferring hasLibrary, which will prevent is! GetterSetterCombo below from promoting.)
| /// The [Library] of a model can be `null` in three cases: | ||
| /// | ||
| /// * the library for `dynamic` and `Never`. | ||
| /// * the library for type parameters. <!-- Is this true? --> |
There was a problem hiding this comment.
It seems Parameter was also called without a library in some tests.
There was a problem hiding this comment.
Yes, I think under this design Parameters are also allowed to have no library.
| e, enclosingContainer, library, packageGraph, | ||
| e, enclosingContainer, library!, packageGraph, | ||
| originalElement: originalMember as ExecutableElement?), | ||
| FormalParameterElement() => Parameter(e, library, packageGraph, |
There was a problem hiding this comment.
Accepts null library too. Was called with it in some tests.
It may just have been called with the sentinel value, and never used it for anything, but it means there is a path that creates such an element.
lib/src/model/model_element.dart
Outdated
| final canonicalLibrary = this.canonicalLibrary; | ||
| var isLibraryOrCanonicalLibraryPrivate = !library.isPublic && | ||
| (canonicalLibrary == null || !canonicalLibrary.isPublic); | ||
| if (library == Library.sentinel || isLibraryOrCanonicalLibraryPrivate) { |
There was a problem hiding this comment.
The test was bugged. The code does library.isPublic above before testing for the sentinel, which means that if it was a sentinel value, it would throw before this test.
| /// The [Library] of a model can be `null` in three cases: | ||
| /// | ||
| /// * the library for `dynamic` and `Never`. | ||
| /// * the library for type parameters. <!-- Is this true? --> |
There was a problem hiding this comment.
Yes, I think under this design Parameters are also allowed to have no library.
| /// The [Library] of a model can be `null` in three cases: | ||
| /// | ||
| /// * the library for `dynamic` and `Never`. | ||
| /// * the library for type parameters. <!-- Is this true? --> |
There was a problem hiding this comment.
Is this true?
I'm not sure if this is actually the case for type parameters. Regardless, I wasn't a fan of the sentinel pattern either, so I think it's a good idea to review the design around library now that we're cleaning this up.
Use `null` instead of a sentinel value that throws on all member accessses. That lets the type system help with avoiding spurious runtime errors when forgetting to check for a sentinel, and it makes it explicit, as a `.library!`, where code assumes that there is a valid library. (Some small drive-by tweaks.)
Small tweaks while merging.
lrhn
force-pushed
the
remove-library-sentinel
branch
from
a2cfca5 to
7998bf8
Compare
Few more tweaks. RegExps are bad, m'kay.
lrhn
force-pushed
the
remove-library-sentinel
branch
from
7998bf8 to
7277503
Compare
3 participants
Use
nullinstead of a sentinel value that throws on all member accessses.That lets the type system help with avoiding spurious runtime errors
if forgetting to check for a sentinel.
It makes it explicit, as a
.library!, where code assumes that there is a valid library.(Some small drive-by tweaks.)