Skip to content

Commit 2d3fc60

Browse files
HosseinYousefiHosseinYousefi
authored
[jnigen] Respect top-type nullability of type arguments (#1909)
1 parent ba34674 commit 2d3fc60

File tree

6 files changed

+152
-107
lines changed

6 files changed

+152
-107
lines changed

‎pkgs/jnigen/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
- Support nullability annotations that are on Java elements like methods and
55
fields instead of directly on the return type or field type.
66
- Fixed a bug where enum values were generated as nullable.
7+
- Fixed a bug where type arguments could be nullable when the top type of their
8+
paramater was non-nullable.
79

810
## 0.13.0
911

‎pkgs/jnigen/lib/src/bindings/dart_generator.dart

Lines changed: 112 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,20 @@ extension on String {
5959
}
6060
}
6161

62+
extension on DeclaredType {
63+
List<T> mapTypeParameters<T>(
64+
T Function(bool isNullable, TypeUsage definedType) mapper,
65+
) {
66+
final result = <T>[];
67+
final offset = classDecl.allTypeParams.length - params.length;
68+
for (var i = 0; i < classDecl.allTypeParams.length; ++i) {
69+
final param = i >= offset ? params[i - offset] : TypeUsage.object;
70+
result.add(mapper(classDecl.allTypeParams[i].isNullable, param));
71+
}
72+
return result;
73+
}
74+
}
75+
6276
String _newLine({int depth = 0}) {
6377
return '\n${' ' * depth}';
6478
}
@@ -732,6 +746,12 @@ $indent/// $comments
732746
class _TypeGenerator extends TypeVisitor<String> {
733747
final Resolver? resolver;
734748

749+
/// Whether the top-type of the current type being visited is nullable.
750+
///
751+
/// For example the top-type of `T` in `Foo<T extends Bar>` is `Bar`, this
752+
/// will be true if `Bar` is nullable.
753+
final bool isTopTypeNullable;
754+
735755
final bool forInterfaceImplementation;
736756

737757
/// Whether the generic types should be erased.
@@ -748,18 +768,21 @@ class _TypeGenerator extends TypeVisitor<String> {
748768
this.typeErasure = false,
749769
this.includeNullability = true,
750770
this.arrayType = false,
771+
this.isTopTypeNullable = true,
751772
});
752773

753774
@override
754775
String visitArrayType(ArrayType node) {
755776
final innerType = node.elementType;
756-
final nullable = includeNullability && node.isNullable ? '?' : '';
777+
final nullable =
778+
includeNullability && node.isNullable && isTopTypeNullable ? '?' : '';
757779
final typeGenerator = _TypeGenerator(
758780
resolver,
759781
forInterfaceImplementation: forInterfaceImplementation,
760782
typeErasure: forInterfaceImplementation,
761783
includeNullability: true,
762784
arrayType: true,
785+
isTopTypeNullable: true,
763786
);
764787
if (innerType.kind == Kind.primitive) {
765788
return '$_jni.J${innerType.accept(typeGenerator)}Array$nullable';
@@ -769,44 +792,25 @@ class _TypeGenerator extends TypeVisitor<String> {
769792

770793
@override
771794
String visitDeclaredType(DeclaredType node) {
772-
final nullable = includeNullability && node.isNullable ? '?' : '';
773-
if (node.classDecl.binaryName == 'java.lang.Object' ||
774-
node.classDecl.binaryName == 'java.lang.String') {
775-
return '$_jni.${node.classDecl.finalName}$nullable';
795+
if (node.classDecl.isObject) {
796+
// The class is not generated, fall back to `JObject`.
797+
return super.visitDeclaredType(node);
776798
}
777-
778-
// All type parameters of this type
779-
final allTypeParams = node.classDecl.allTypeParams
780-
.map((typeParam) => typeParam.name)
781-
.toList();
782-
// The ones that are declared.
783-
final typeGenerator = _TypeGenerator(
784-
resolver,
785-
forInterfaceImplementation: forInterfaceImplementation,
786-
typeErasure: forInterfaceImplementation,
787-
includeNullability: true,
788-
arrayType: false,
799+
final nullable =
800+
includeNullability && node.isNullable && isTopTypeNullable ? '?' : '';
801+
802+
final allTypeParams = node.mapTypeParameters(
803+
(isNullable, definedType) {
804+
return definedType.accept(_TypeGenerator(
805+
resolver,
806+
forInterfaceImplementation: forInterfaceImplementation,
807+
typeErasure: forInterfaceImplementation,
808+
includeNullability: true,
809+
arrayType: false,
810+
isTopTypeNullable: isNullable,
811+
));
812+
},
789813
);
790-
final definedTypeParams = node.params.accept(typeGenerator).toList();
791-
792-
// Replacing the declared ones. They come at the end.
793-
// The rest will be JObject.
794-
if (allTypeParams.length >= node.params.length) {
795-
final nullable = includeNullability ? '?' : '';
796-
allTypeParams.replaceRange(
797-
0,
798-
allTypeParams.length - node.params.length,
799-
List.filled(
800-
allTypeParams.length - node.params.length,
801-
'$_jObject$nullable',
802-
),
803-
);
804-
allTypeParams.replaceRange(
805-
allTypeParams.length - node.params.length,
806-
allTypeParams.length,
807-
definedTypeParams,
808-
);
809-
}
810814

811815
final typeParams = allTypeParams.join(', ').encloseIfNotEmpty('<', '>');
812816
final prefix = resolver?.resolvePrefix(node.classDecl) ?? '';
@@ -826,7 +830,8 @@ class _TypeGenerator extends TypeVisitor<String> {
826830
// TODO(https://github.com/dart-lang/native/issues/704): Tighten to
827831
// typevar bounds instead.
828832
{
829-
final nullable = includeNullability && node.isNullable ? '?' : '';
833+
final nullable =
834+
includeNullability && node.isNullable && isTopTypeNullable ? '?' : '';
830835
if (typeErasure) {
831836
return '$_jObject$nullable';
832837
}
@@ -849,15 +854,18 @@ class _TypeGenerator extends TypeVisitor<String> {
849854
resolver,
850855
arrayType: arrayType,
851856
forInterfaceImplementation: forInterfaceImplementation,
852-
includeNullability: includeNullability && node.isNullable,
857+
includeNullability:
858+
includeNullability && node.isNullable && isTopTypeNullable,
853859
typeErasure: typeErasure,
860+
isTopTypeNullable: true,
854861
);
855862
return node.extendsBound!.accept(typeGenerator);
856863
}
857864

858865
@override
859866
String visitNonPrimitiveType(ReferredType node) {
860-
final nullable = includeNullability ? '?' : '';
867+
final nullable =
868+
includeNullability && isTopTypeNullable && node.isNullable ? '?' : '';
861869
return '$_jObject$nullable';
862870
}
863871
}
@@ -873,6 +881,12 @@ class _TypeClass {
873881
class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
874882
final bool isConst;
875883

884+
/// Whether the top-type of the current type being visited is nullable.
885+
///
886+
/// For example the top-type of `T` in `Foo<T extends Bar>` is `Bar`, this
887+
/// will be true if `Bar` is nullable.
888+
final bool isTopTypeNullable;
889+
876890
/// Whether or not to return the equivalent boxed type class for primitives.
877891
/// Only for interface implemetation.
878892
final bool boxPrimitives;
@@ -895,6 +909,7 @@ class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
895909
this.forInterfaceImplementation = false,
896910
this.includeNullability = true,
897911
this.typeErasure = false,
912+
this.isTopTypeNullable = true,
898913
});
899914

900915
@override
@@ -907,6 +922,7 @@ class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
907922
forInterfaceImplementation: forInterfaceImplementation,
908923
// Do type erasure for interface implementation.
909924
typeErasure: forInterfaceImplementation,
925+
isTopTypeNullable: true,
910926
),
911927
);
912928
final innerType = node.elementType.accept(
@@ -916,11 +932,13 @@ class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
916932
// Do type erasure for interface implementation.
917933
typeErasure: forInterfaceImplementation,
918934
arrayType: true,
935+
isTopTypeNullable: true,
919936
),
920937
);
921938
final ifConst = innerTypeClass.canBeConst && isConst ? 'const ' : '';
922-
final type =
923-
includeNullability && node.isNullable ? 'NullableType' : 'Type';
939+
final type = includeNullability && node.isNullable && isTopTypeNullable
940+
? 'NullableType'
941+
: 'Type';
924942
if (node.elementType.kind == Kind.primitive) {
925943
return _TypeClass(
926944
'$ifConst$_jni.J${innerType}Array$type()',
@@ -935,68 +953,54 @@ class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
935953

936954
@override
937955
_TypeClass visitDeclaredType(DeclaredType node) {
938-
final allTypeParams = node.classDecl.allTypeParams
939-
.map((typeParam) => typeParam.name)
940-
.toList();
941-
942-
// The ones that are declared.
943-
final definedTypeClasses = node.params.accept(
944-
_TypeClassGenerator(
945-
resolver,
946-
isConst: false,
947-
boxPrimitives: false,
948-
forInterfaceImplementation: forInterfaceImplementation,
949-
typeErasure: forInterfaceImplementation,
950-
),
956+
if (node.classDecl.isObject) {
957+
// The class is not generated, fall back to `JObject`.
958+
return super.visitDeclaredType(node);
959+
}
960+
final allTypeClasses = node.mapTypeParameters(
961+
(isNullable, definedType) {
962+
return definedType.accept(_TypeClassGenerator(
963+
resolver,
964+
isConst: false,
965+
boxPrimitives: false,
966+
forInterfaceImplementation: forInterfaceImplementation,
967+
typeErasure: forInterfaceImplementation,
968+
isTopTypeNullable: isNullable,
969+
));
970+
},
951971
);
952972

953973
// Can be const if all the type parameters are defined and each of them are
954974
// also const.
955-
final canBeConst =
956-
allTypeParams.isEmpty || definedTypeClasses.every((e) => e.canBeConst);
975+
final canBeConst = allTypeClasses.every((e) => e.canBeConst);
957976

958-
// Replacing the declared ones. They come at the end.
959-
// The rest will be `JObjectNullableType`.
960-
if (allTypeParams.length >= node.params.length) {
961-
allTypeParams.replaceRange(
962-
0,
963-
allTypeParams.length - node.params.length,
964-
List.filled(
965-
allTypeParams.length - node.params.length,
966-
// Adding const to subexpressions if the entire expression is not
967-
// const.
968-
'${canBeConst ? '' : 'const '}${_jObject}NullableType()',
969-
),
970-
);
971-
allTypeParams.replaceRange(
972-
allTypeParams.length - node.params.length,
973-
allTypeParams.length,
974-
// Adding const to subexpressions if the entire expression is not const.
975-
definedTypeClasses.map(
976-
(param) =>
977-
'${param.canBeConst && !canBeConst ? 'const ' : ''}${param.name}',
978-
),
979-
);
980-
}
977+
// Add const to subexpressions if the entire expression is not const.
978+
final allTypeParams = allTypeClasses
979+
.map((typeClass) =>
980+
'${typeClass.canBeConst && !canBeConst ? 'const ' : ''}'
981+
'${typeClass.name}')
982+
.toList();
981983

982984
final args = allTypeParams.join(', ');
983985
final ifConst = isConst && canBeConst ? 'const ' : '';
984-
final type = includeNullability && node.isNullable
986+
final type = includeNullability && node.isNullable && isTopTypeNullable
985987
? node.classDecl.nullableTypeClassName
986988
: node.classDecl.typeClassName;
987-
final typeArgs = node.classDecl.isObject
988-
? ''
989-
: node.params
990-
.accept(
991-
_TypeGenerator(
992-
resolver,
993-
forInterfaceImplementation: forInterfaceImplementation,
994-
// Do type erasure for interface implementation.
995-
typeErasure: forInterfaceImplementation,
996-
),
997-
)
998-
.join(', ')
999-
.encloseIfNotEmpty('<', '>');
989+
990+
final typeArgsList = node.mapTypeParameters(
991+
(isNullable, definedType) {
992+
return definedType.accept(
993+
_TypeGenerator(
994+
resolver,
995+
forInterfaceImplementation: forInterfaceImplementation,
996+
// Do type erasure for interface implementation.
997+
typeErasure: forInterfaceImplementation,
998+
isTopTypeNullable: isNullable,
999+
),
1000+
);
1001+
},
1002+
);
1003+
final typeArgs = typeArgsList.join(', ').encloseIfNotEmpty('<', '>');
10001004
final prefix = resolver.resolvePrefix(node.classDecl);
10011005
return _TypeClass('$ifConst$prefix$type$typeArgs($args)', canBeConst);
10021006
}
@@ -1012,10 +1016,13 @@ class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
10121016
_TypeClass visitTypeVar(TypeVar node) {
10131017
// TODO(https://github.com/dart-lang/native/issues/704): Tighten to typevar
10141018
// bounds instead.
1015-
final type =
1016-
includeNullability && node.hasQuestionMark ? 'NullableType' : 'Type';
1019+
final type = includeNullability && node.hasQuestionMark && isTopTypeNullable
1020+
? 'NullableType'
1021+
: 'Type';
10171022
final convertToNullable =
1018-
includeNullability && node.hasQuestionMark ? '.nullableType' : '';
1023+
includeNullability && node.hasQuestionMark && isTopTypeNullable
1024+
? '.nullableType'
1025+
: '';
10191026
if (typeErasure) {
10201027
final ifConst = isConst ? 'const ' : '';
10211028
return _TypeClass('$ifConst$_jObject$type()', true);
@@ -1044,18 +1051,21 @@ class _TypeClassGenerator extends TypeVisitor<_TypeClass> {
10441051
resolver,
10451052
boxPrimitives: boxPrimitives,
10461053
forInterfaceImplementation: forInterfaceImplementation,
1047-
includeNullability: includeNullability && node.isNullable,
1054+
includeNullability:
1055+
includeNullability && node.isNullable && isTopTypeNullable,
10481056
isConst: isConst,
10491057
typeErasure: typeErasure,
1058+
isTopTypeNullable: true,
10501059
);
10511060
return node.extendsBound!.accept(typeClassGenerator);
10521061
}
10531062

10541063
@override
10551064
_TypeClass visitNonPrimitiveType(ReferredType node) {
10561065
final ifConst = isConst ? 'const ' : '';
1057-
final type =
1058-
includeNullability && node.isNullable ? 'NullableType' : 'Type';
1066+
final type = includeNullability && node.isNullable && isTopTypeNullable
1067+
? 'NullableType'
1068+
: 'Type';
10591069
return _TypeClass('$ifConst$_jObject$type()', true);
10601070
}
10611071
}
@@ -1667,7 +1677,8 @@ class _TypeVarLocator extends TypeVisitor<Map<String, List<OutsideInBuffer>>> {
16671677
@override
16681678
Map<String, List<OutsideInBuffer>> visitDeclaredType(DeclaredType node) {
16691679
if (node.classDecl.isObject) {
1670-
return {};
1680+
// The class is not generated, fall back to `JObject`.
1681+
return super.visitDeclaredType(node);
16711682
}
16721683
final offset = node.classDecl.allTypeParams.length - node.params.length;
16731684
final result = <String, List<OutsideInBuffer>>{};

‎pkgs/jnigen/lib/src/bindings/linker.dart

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ class Linker extends Visitor<Classes, Future<void>> {
6666
resolve(TypeUsage.object.name);
6767
}
6868

69+
(TypeUsage.object.type as DeclaredType).classDecl =
70+
resolve(TypeUsage.object.name);
6971
final classLinker = _ClassLinker(
7072
config,
7173
resolve,
@@ -230,10 +232,10 @@ class _TypeLinker extends TypeVisitor<void> {
230232

231233
@override
232234
void visitDeclaredType(DeclaredType node) {
235+
node.classDecl = resolve(node.binaryName);
233236
for (final param in node.params) {
234237
param.accept(this);
235238
}
236-
node.classDecl = resolve(node.binaryName);
237239
}
238240

239241
@override

‎pkgs/jnigen/lib/src/elements/elements.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ class TypeUsage {
223223
required this.typeJson,
224224
});
225225

226-
static TypeUsage object = TypeUsage(
226+
static final object = TypeUsage(
227227
kind: Kind.declared, shorthand: 'java.lang.Object', typeJson: {})
228228
..type = DeclaredType(binaryName: 'java.lang.Object');
229229

0 commit comments

Comments
 (0)