dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.11k stars 1.57k forks source link

[docs/ffi] `.address` position should mention `.cast()` #56613

Open codesculpture opened 1 month ago

codesculpture commented 1 month ago

TODO


Original bug report:

While trying to fix this, I found the below code is

@Native<Void Function(Pointer<Void>)>(isLeaf: true)
external void myNative(Pointer<Void> buff);

void main() {
  final buff = Int8List.fromList([2]);
  myNative(buff.address.cc);
}

throws 2 errors by the analyzer

1. ERROR|COMPILE_TIME_ERROR|ADDRESS_POSITION|9|17|7|The '.address' expression can only be used as argument to a leaf native external call.
2. ERROR|COMPILE_TIME_ERROR|UNDEFINED_GETTER|9|25|2|The getter 'cc' isn't defined for the type 'Pointer<Int8>'.

But expected is only one error

1. ERROR|COMPILE_TIME_ERROR|UNDEFINED_GETTER|9|25|2|The getter 'cc' isn't defined for the type 'Pointer<Int8>'.

FYI @dcharkes

dart-github-bot commented 1 month ago

Summary: The analyzer incorrectly throws an .address position error when a native call is made with an undefined getter, even though the getter error should be the only one reported. This issue was discovered while trying to fix a related issue.

codesculpture commented 1 month ago

I found the root cause of this issue and i actually tried to fix this, but i planned to fix this seperately once i landed this

https://github.com/dart-lang/sdk/issues/56462

codesculpture commented 1 month ago

(am not good at writing issue title, feel free to edit)

codesculpture commented 1 month ago

I had a thought while reasoning about this issue, we recently "supported" .cast() on Pointers here

But i think thats a existing "bug" which we just fixed (i.e its not something we added to support, it should have worked before)

Because as far as i understand "Address Position" Error, We are only allowing .address on leaf native call.

From the above statement, .address.cast() should have worked before since still the .address is called on native leaf call.

Also we should allow .address.a.b.c these on native leaf call, if such a,b,c exists. The cfe is now allowing such expression, this been landed while fixing this https://github.com/dart-lang/sdk/issues/56462 So now analyzer should do the same.

If we dont allow such these expression address.a.b.c, then the definition of error should be different from what we have already "Address Position", it should be more like "Address member access is prohibited except cast" But i dont think this is what we intended to.

Feel free to correct me. @dcharkes

codesculpture commented 1 month ago

Requested CL (this is my first direct cl which was created through git cl upload, previously copybara helped me) here, based on this below statement

Also we should allow .address.a.b.c these on native leaf call.

dcharkes commented 1 month ago

If we dont allow such these expression address.a.b.c, then the definition of error should be different from what we have already "Address Position", it should be more like "Address member access is prohibited except cast"

It should be like this. .address is not a real expression, it changes how the FFI call works.

dcharkes commented 1 month ago

throws 2 errors by the analyzer

1. ERROR|COMPILE_TIME_ERROR|ADDRESS_POSITION|9|17|7|The '.address' expression can only be used as argument to a leaf native external call.
2. ERROR|COMPILE_TIME_ERROR|UNDEFINED_GETTER|9|25|2|The getter 'cc' isn't defined for the type 'Pointer<Int8>'.

But expected is only one error

1. ERROR|COMPILE_TIME_ERROR|UNDEFINED_GETTER|9|25|2|The getter 'cc' isn't defined for the type 'Pointer<Int8>'.

I think we would expect two errors here. They are separate errors, not one error causing the next error. Pointer doesn't have a .cc field. And .address can only be the last expression (except for cast) for arguments of a leaf call.

You could compare it for example to trying to call a private method in Java or C++ and passing the wrong argument type to that method. You'd get an error that the method is private, and that the argument type is wrong.

I think ADDRESS_POSITION message should maybe be updated to say The '.address' expression can only be used as argument to a leaf native external call. The '.address' expression must be the direct argument or be used as '.address.cast()'.

Or maybe it should not be in the error message itself, but in the dartdoc comment of .address. And it should then also be in the published docs for the analyzer message @MaryaBelanger.

codesculpture commented 1 month ago

Well, now it makes sense. Then analyzer is doing the right job, but cfe is the one still buggy. We can close this and track the cfe issue in new issue ticket? @dcharkes

codesculpture commented 1 month ago

Thanks @dcharkes for clearing this, i think updating this error message would clearly indicate its purpose.

dcharkes commented 1 month ago

Well, now it makes sense. Then analyzer is doing the right job, but cfe is the one still buggy.

You can just make a PR without a bug report if you want to work on it right away. It saves who follow the bug reports on this repository an email. 😉

We can close this and track the cfe issue in new issue ticket?

We can update this issue to make it track updating the docs.

codesculpture commented 1 month ago

Btw, am just trying to understand the issue, If we really dont want anyone to access Pointer's address (which is int), or any members (except cast), why cant we just make them as private or if not used internally we can remove them. I understand it somehow used, but am not sure how.

Any previous references or issue regarding this also helps me to understand.

dcharkes commented 4 weeks ago

Btw, am just trying to understand the issue, If we really dont want anyone to access Pointer's address (which is int), or any members (except cast), why cant we just make them as private or if not used internally we can remove them. I understand it somehow used, but am not sure how.

Any previous references or issue regarding this also helps me to understand.

You can find references in the following way:

codesculpture commented 4 weeks ago

@dcharkes

When constructing new Arguments(listOfExpressions), where every expression's location in listOfExpression is being set to null after the construction of Arguments, is there any way to hold the location of every expression even if its constructed for a new Arguments

dcharkes commented 4 weeks ago

@dcharkes

When constructing new Arguments(listOfExpressions), where every expression's location in listOfExpression is being set to null after the construction of Arguments, is there any way to hold the location of every expression even if its constructed for a new Arguments

Where is it being set to null? Why is it being set to null? Please add a reference to the code that does it.

codesculpture commented 3 weeks ago

Here https://github.com/dart-lang/sdk/blob/main/pkg%2Fvm%2Flib%2Fmodular%2Ftransformations%2Fffi%2Fuse_sites.dart#L1485-L1488

newArguments's every element (Expression) has location, but after it passed to construct Arguments its been just set to null, and i see that Arguments constructor is re-assigning parent of every argument initially in its implementation, i just dived this much. Just in case if it helps.

@dcharkes

I faced consequence of this as an issue while reporting cfe error, where after this transformation, node.location?.file where the file is being null, thus error cannot point the file which belongs to.

Here https://github.com/dart-lang/sdk/blob/main/pkg%2Fvm%2Flib%2Fmodular%2Ftransformations%2Fffi%2Fuse_sites.dart#L538-L539

dcharkes commented 3 weeks ago

newArguments's every element (Expression) has location, but after it passed to construct Arguments its been just set to null, and i see that Arguments constructor is re-assigning parent of every argument initially in its implementation, i just dived this much. Just in case if it helps.

Maybe @jensjoha or @chloestefantsova knows the answer to this.

codesculpture commented 3 weeks ago

Seems location is not intentionally set to "null", but it is a getter which will get its location based on its parent, but Arguments constructor is resetting its parent and then it might considered to be a orphan node, hence no location can be derived?

codesculpture commented 3 weeks ago

Yes, here https://github.com/dart-lang/sdk/blob/main/pkg%2Fvm%2Flib%2Fmodular%2Ftransformations%2Fffi%2Fuse_sites.dart#L1485-L1488

The constructed StaticInvocation is orphan while walking here https://github.com/dart-lang/sdk/blob/main/pkg%2Fvm%2Flib%2Fmodular%2Ftransformations%2Fffi%2Fuse_sites.dart#L538-L539 which makes sense.

codesculpture commented 3 weeks ago

https://dart-review.googlesource.com/c/sdk/+/384080 @dcharkes

codesculpture commented 2 days ago

Hi @dcharkes, Any update on updating docs?