dart:ffi analysis rules in dart analyzer #35777
Labels
area-core-library
SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries.
library-ffi
Comments
kevmoo
added
the
area-core-library
|
Are we absolutely sure we cannot encode these type requirements in the Dart types somehow, rather than introduce a completely new type system that the analyzer needs to know about? I can see that it is annoying to have to write typedef Int32p = Pointer<Int32, int>;
typedef Int16p = Pointer<Int16, int>;
typedef Float64p = Pointer<Float64, double>;Or, the type class CType<T> {
Pointer<CType<T>, T> get NULL;
}
class Int32 extends CType<int> {
Pointer<Int32, int> get NULL => Pointer<Int32, int>.NULL();
}
class Pointer<C extends CType<T>, T> {
Pointer.NULL() : ...;
}and then you can write: to initialize a pointer to the full type without having to write it. |
|
@MichaelRFairhurst @stereotype441 @bwilkerson @scheglov Is this the sort of thing that might best be done in a plugin? What would be involved in that? |
|
As discussed last meeting, assigning @sjindel-google. |
|
Note that we are not entirely sure that the performance of plugins is good enough for plugins in the current design. Definitely the first option that should be explored is using the dart type checker normally. |
Could you please elaborate on this? I was planning to pursue implementing this as hints, given @bwilkerson's feedback on dart-lang/linter#1734. |
|
by "using the dart type checker normally" I meant like @lrhn's suggestion. Mostly my comment was in terms of whether we should explore using an analysis plugin. I do not recommend an analysis plugin as this purpose unless all other avenues have been seriously considered. Looks like lints fall into that category of "all other avenues." Though lints have problems too...even if they are enabled by default, it is not ideal to have hints in the analyzer for something that is a compile time error in the CFE. But I'm assuming that these trade-offs have already been thought through pretty thoroughly. |
|
tl;dr Implementing these diagnostics as hints is the best implementation strategy. I agree that we don't want to use a plugin for these. In addition to the potential performance issues, we want these to always be enabled. Plugins are like lints in that respect because users have to add a dependency to their package in order to allow the plugin to be loaded. Note that a hint is just a diagnostic that isn't defined by the Dart Language Specification. The severity of the hint can be set to "error", if that's what we want, and users will see it exactly the same was as any of the diagnostics defined in the spec. |
The simple cases with extension PointerInt8 on Pointer<Int8> {
int load();
store(int v);
}
// etc.The things which cannot be done in the Dart type system is pairwise checking types in two function signatures for an arbitrary length signature: Pointer<NativeFunction<Int8 Function(Int8, Int8)>> p;
final f = p.asFunction<int Function(int, int)>();I thought @lrhn, @sjindel-google, and I had discussed that (half a year ago), but I cannot find the issue/post on GitHub. (Maybe it was on a whiteboard.) |
|
There are also non-type-system related checks, like:
However, we should be using |
|
We're enabling the analyzer to run on our
|
dart-bot
pushed a commit
that referenced
this issue
Oct 10, 2019
Issue #35777 Change-Id: Ia8edd9901d52e671a116c7747319fdadd7da4681 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121060 Reviewed-by: Daco Harkes <dacoharkes@google.com> Reviewed-by: Samir Jindel <sjindel@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>
|
@bwilkerson Thank you so much for helping us on this - very much appreciate it! |
|
@mkustermann I've preapproved the following test as well.
|
|
I've uploaded cl/121711 with the remaining changes to the analyzer, which should make the analyzer pass our |
dart-bot
pushed a commit
that referenced
this issue
Oct 15, 2019
This makes all tests in the ffi test suite pass: % tools/test.py -cdart2analyzer -mrelease -ax64 ffi Issue #35777 Change-Id: I93338ee530041e5e8cb1eb5958b12fbf1517496e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121711 Auto-Submit: Martin Kustermann <kustermann@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
|
The main checks have been implemented. We'll probably want to fine tune the error messages to make CFE and Analyzer provide roughly the same error message. Though I don't consider this blocking for D26. So removing the milestone label. |
|
Filed #38970 for the alignment of error messages. We consider this bug to be fixed - the analyzer has FFI support now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The Dart compiler rejects programs
dart:ffiif the C and Dart types do not match up:However, the Dart analyzer does not report these errors yet.
Implement the same static checks in Dart analyzer.
The text was updated successfully, but these errors were encountered: