From be195ad329fd05ca7a3d3085fb391e6dfbd1f649 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Fri, 15 Oct 2021 11:21:23 +0000 Subject: [PATCH] [analyzer/ffi] Report non-const `fromFunction` argument * Adds missing error to analyzer. * Adds analyzer test. * Adds analyzer/cfe negative test. * Formats the analyzer/cfe negative tests file. TEST=pkg/analyzer/lib/src/generated/ffi_verifier.dart TEST=tests/ffi/vmspecific_static_checks_test.dart Bug: https://github.com/dart-lang/sdk/issues/36780 Change-Id: Ibc2a3d1c3b4b4d8b2285605f084237a55253fb20 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/217007 Commit-Queue: Daco Harkes Reviewed-by: Clement Skau --- .../lib/src/generated/ffi_verifier.dart | 36 +++++++++++++++---- .../argument_must_be_a_constant_test.dart | 14 ++++++++ tests/ffi/vmspecific_static_checks_test.dart | 34 +++++++++++++----- .../ffi_2/vmspecific_static_checks_test.dart | 34 +++++++++++++----- 4 files changed, 95 insertions(+), 23 deletions(-) diff --git a/pkg/analyzer/lib/src/generated/ffi_verifier.dart b/pkg/analyzer/lib/src/generated/ffi_verifier.dart index af80e94b395..df2a706f1c8 100644 --- a/pkg/analyzer/lib/src/generated/ffi_verifier.dart +++ b/pkg/analyzer/lib/src/generated/ffi_verifier.dart @@ -407,6 +407,24 @@ class FfiVerifier extends RecursiveAstVisitor { return false; } + bool _isConst(Expression expr) { + if (expr is Literal) { + return true; + } + if (expr is Identifier) { + final staticElm = expr.staticElement; + if (staticElm is ConstVariableElement) { + return true; + } + if (staticElm is PropertyAccessorElementImpl) { + if (staticElm.variable is ConstVariableElement) { + return true; + } + } + } + return false; + } + /// Returns `true` if [nativeType] is a C type that has a size. bool _isSized(DartType nativeType) { switch (_primitiveNativeType(nativeType)) { @@ -510,12 +528,13 @@ class FfiVerifier extends RecursiveAstVisitor { return false; } - // Get the const bool value of `expr` if it exists. - // Return null if it isn't a const bool. + /// Get the const bool value of [expr] if it exists. + /// Return null if it isn't a const bool. bool? _maybeGetBoolConstValue(Expression expr) { if (expr is BooleanLiteral) { return expr.value; - } else if (expr is Identifier) { + } + if (expr is Identifier) { final staticElm = expr.staticElement; if (staticElm is ConstVariableElement) { return staticElm.computeConstantValue()?.toBoolValue(); @@ -897,23 +916,26 @@ class FfiVerifier extends RecursiveAstVisitor { FfiCode.MISSING_EXCEPTION_VALUE, node.methodName); } else { Expression e = node.argumentList.arguments[1]; - // TODO(brianwilkerson) Validate that `e` is a constant expression. if (!_validateCompatibleNativeType(e.typeOrThrow, R, true)) { _errorReporter.reportErrorForNode( FfiCode.MUST_BE_A_SUBTYPE, e, [e.staticType, R, 'fromFunction']); } + if (!_isConst(e)) { + _errorReporter.reportErrorForNode( + FfiCode.ARGUMENT_MUST_BE_A_CONSTANT, e, ['exceptionalReturn']); + } } } + /// Ensure `isLeaf` is const as we need the value at compile time to know + /// which trampoline to generate. void _validateIsLeafIsConst(MethodInvocation node) { - // Ensure `isLeaf` is const as we need the value at compile time to know - // which trampoline to generate. final args = node.argumentList.arguments; if (args.isNotEmpty) { for (final arg in args) { if (arg is NamedExpression) { if (arg.element?.name == _isLeafParamName) { - if (_maybeGetBoolConstValue(arg.expression) == null) { + if (!_isConst(arg.expression)) { _errorReporter.reportErrorForNode( FfiCode.ARGUMENT_MUST_BE_A_CONSTANT, arg.expression, diff --git a/pkg/analyzer/test/src/diagnostics/argument_must_be_a_constant_test.dart b/pkg/analyzer/test/src/diagnostics/argument_must_be_a_constant_test.dart index 8db3c127891..61dd5710ca7 100644 --- a/pkg/analyzer/test/src/diagnostics/argument_must_be_a_constant_test.dart +++ b/pkg/analyzer/test/src/diagnostics/argument_must_be_a_constant_test.dart @@ -62,6 +62,20 @@ doThings(bool isLeaf) { ]); } + test_FromFunctionExceptionReturn() async { + await assertErrorsInCode(r''' +import 'dart:ffi'; +typedef NativeDoubleUnOp = Double Function(Double); +double myTimesThree(double d) => d * 3; +void testFromFunctionFunctionExceptionValueMustBeConst() { + final notAConst = 1.1; + Pointer.fromFunction(myTimesThree, notAConst); +} +''', [ + error(FfiCode.ARGUMENT_MUST_BE_A_CONSTANT, 250, 9), + ]); + } + test_LookupFunctionIsLeaf() async { await assertErrorsInCode(r''' import 'dart:ffi'; diff --git a/tests/ffi/vmspecific_static_checks_test.dart b/tests/ffi/vmspecific_static_checks_test.dart index 9dca0e71aa1..e77b9417986 100644 --- a/tests/ffi/vmspecific_static_checks_test.dart +++ b/tests/ffi/vmspecific_static_checks_test.dart @@ -36,6 +36,7 @@ void main() { testFromFunctionClosure(); testFromFunctionTearOff(); testFromFunctionAbstract(); + testFromFunctionFunctionExceptionValueMustBeConst(); testLookupFunctionGeneric(); testLookupFunctionGeneric2(); testLookupFunctionWrongNativeFunctionSignature(); @@ -274,6 +275,12 @@ void testFromFunctionAbstract() { testFromFunctionAbstract); //# 76: compile-time error } +void testFromFunctionFunctionExceptionValueMustBeConst() { + final notAConst = 1.1; + Pointer> p; + p = Pointer.fromFunction(myTimesThree, notAConst); //# 77: compile-time error +} + void testLookupFunctionGeneric() { Function generic() { DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); @@ -714,13 +721,16 @@ class TestStruct1405 extends Struct { void testLookupFunctionIsLeafMustBeConst() { bool notAConst = false; DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); - l.lookupFunction("timesFour", isLeaf:notAConst); //# 1500: compile-time error + l.lookupFunction< //# 1500: compile-time error + NativeDoubleUnOp, //# 1500: compile-time error + DoubleUnOp>("timesFour", //# 1500: compile-time error + isLeaf: notAConst); //# 1500: compile-time error } void testAsFunctionIsLeafMustBeConst() { bool notAConst = false; Pointer> p = Pointer.fromAddress(1337); - IntUnOp f = p.asFunction(isLeaf:notAConst); //# 1501: compile-time error + IntUnOp f = p.asFunction(isLeaf: notAConst); //# 1501: compile-time error } typedef NativeTakesHandle = Void Function(Handle); @@ -728,12 +738,16 @@ typedef TakesHandle = void Function(Object); void testLookupFunctionTakesHandle() { DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); - l.lookupFunction("takesHandle", isLeaf:true); //# 1502: compile-time error + l.lookupFunction< //# 1502: compile-time error + NativeTakesHandle, //# 1502: compile-time error + TakesHandle>("takesHandle", //# 1502: compile-time error + isLeaf: true); //# 1502: compile-time error } void testAsFunctionTakesHandle() { - Pointer> p = Pointer.fromAddress(1337); //# 1503: compile-time error - TakesHandle f = p.asFunction(isLeaf:true); //# 1503: compile-time error + Pointer> p = //# 1503: compile-time error + Pointer.fromAddress(1337); //# 1503: compile-time error + TakesHandle f = p.asFunction(isLeaf: true); //# 1503: compile-time error } typedef NativeReturnsHandle = Handle Function(); @@ -741,12 +755,16 @@ typedef ReturnsHandle = Object Function(); void testLookupFunctionReturnsHandle() { DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); - l.lookupFunction("returnsHandle", isLeaf:true); //# 1504: compile-time error + l.lookupFunction< //# 1504: compile-time error + NativeReturnsHandle, //# 1504: compile-time error + ReturnsHandle>("returnsHandle", //# 1504: compile-time error + isLeaf: true); //# 1504: compile-time error } void testAsFunctionReturnsHandle() { - Pointer> p = Pointer.fromAddress(1337); //# 1505: compile-time error - ReturnsHandle f = p.asFunction(isLeaf:true); //# 1505: compile-time error + Pointer> p = //# 1505: compile-time error + Pointer.fromAddress(1337); //# 1505: compile-time error + ReturnsHandle f = p.asFunction(isLeaf: true); //# 1505: compile-time error } @Packed(1) diff --git a/tests/ffi_2/vmspecific_static_checks_test.dart b/tests/ffi_2/vmspecific_static_checks_test.dart index 112455c40a3..165b25cd045 100644 --- a/tests/ffi_2/vmspecific_static_checks_test.dart +++ b/tests/ffi_2/vmspecific_static_checks_test.dart @@ -38,6 +38,7 @@ void main() { testFromFunctionClosure(); testFromFunctionTearOff(); testFromFunctionAbstract(); + testFromFunctionFunctionExceptionValueMustBeConst(); testLookupFunctionGeneric(); testLookupFunctionGeneric2(); testLookupFunctionWrongNativeFunctionSignature(); @@ -276,6 +277,12 @@ void testFromFunctionAbstract() { testFromFunctionAbstract); //# 76: compile-time error } +void testFromFunctionFunctionExceptionValueMustBeConst() { + final notAConst = 1.1; + Pointer> p; + p = Pointer.fromFunction(myTimesThree, notAConst); //# 77: compile-time error +} + void testLookupFunctionGeneric() { Function generic() { DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); @@ -714,13 +721,16 @@ class TestStruct1405 extends Struct { void testLookupFunctionIsLeafMustBeConst() { bool notAConst = false; DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); - l.lookupFunction("timesFour", isLeaf:notAConst); //# 1500: compile-time error + l.lookupFunction< //# 1500: compile-time error + NativeDoubleUnOp, //# 1500: compile-time error + DoubleUnOp>("timesFour", //# 1500: compile-time error + isLeaf: notAConst); //# 1500: compile-time error } void testAsFunctionIsLeafMustBeConst() { bool notAConst = false; Pointer> p = Pointer.fromAddress(1337); - IntUnOp f = p.asFunction(isLeaf:notAConst); //# 1501: compile-time error + IntUnOp f = p.asFunction(isLeaf: notAConst); //# 1501: compile-time error } typedef NativeTakesHandle = Void Function(Handle); @@ -728,12 +738,16 @@ typedef TakesHandle = void Function(Object); void testLookupFunctionTakesHandle() { DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); - l.lookupFunction("takesHandle", isLeaf:true); //# 1502: compile-time error + l.lookupFunction< //# 1502: compile-time error + NativeTakesHandle, //# 1502: compile-time error + TakesHandle>("takesHandle", //# 1502: compile-time error + isLeaf: true); //# 1502: compile-time error } void testAsFunctionTakesHandle() { - Pointer> p = Pointer.fromAddress(1337); //# 1503: compile-time error - TakesHandle f = p.asFunction(isLeaf:true); //# 1503: compile-time error + Pointer> p = //# 1503: compile-time error + Pointer.fromAddress(1337); //# 1503: compile-time error + TakesHandle f = p.asFunction(isLeaf: true); //# 1503: compile-time error } typedef NativeReturnsHandle = Handle Function(); @@ -741,12 +755,16 @@ typedef ReturnsHandle = Object Function(); void testLookupFunctionReturnsHandle() { DynamicLibrary l = dlopenPlatformSpecific("ffi_test_dynamic_library"); - l.lookupFunction("returnsHandle", isLeaf:true); //# 1504: compile-time error + l.lookupFunction< //# 1504: compile-time error + NativeReturnsHandle, //# 1504: compile-time error + ReturnsHandle>("returnsHandle", //# 1504: compile-time error + isLeaf: true); //# 1504: compile-time error } void testAsFunctionReturnsHandle() { - Pointer> p = Pointer.fromAddress(1337); //# 1505: compile-time error - ReturnsHandle f = p.asFunction(isLeaf:true); //# 1505: compile-time error + Pointer> p = //# 1505: compile-time error + Pointer.fromAddress(1337); //# 1505: compile-time error + ReturnsHandle f = p.asFunction(isLeaf: true); //# 1505: compile-time error } @Packed(1)