Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks
codesculpture opened this issue Aug 30, 2024 · 20 comments
Open
2 tasks

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

codesculpture opened this issue Aug 30, 2024 · 20 comments
Assignees
Labels
area-documentation Prefer using 'type-documentation' and a specific area label. library-ffi P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@codesculpture
Copy link
Contributor

codesculpture commented Aug 30, 2024

TODO

  • The dartdoc comment of .address should mention or show an example of using .cast().
  • The published ADDRESS_POSITION documentation should mention .cast().

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
Copy link
Collaborator

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.

@dart-github-bot dart-github-bot added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Aug 30, 2024
@codesculpture
Copy link
Contributor Author

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

#56462

@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Aug 30, 2024
@codesculpture codesculpture changed the title [analyzer/ffi]: Analyzer throws .address position error for native call if called with undefined getters [analyzer/ffi] Analyzer throws .address position error for native call if called with undefined getters Aug 30, 2024
@codesculpture codesculpture changed the title [analyzer/ffi] Analyzer throws .address position error for native call if called with undefined getters [analyzer/ffi] Analyzer throws .address position error for native leaf call if called with undefined getters Aug 30, 2024
@codesculpture
Copy link
Contributor Author

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

@scheglov scheglov added the P3 A lower priority bug or feature request label Aug 30, 2024
@codesculpture
Copy link
Contributor Author

codesculpture commented Aug 31, 2024

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 #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
Copy link
Contributor Author

codesculpture commented Aug 31, 2024

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
Copy link
Contributor

dcharkes commented Sep 2, 2024

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
Copy link
Contributor

dcharkes commented Sep 2, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@dcharkes
Copy link
Contributor

dcharkes commented Sep 2, 2024

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.

@dcharkes dcharkes changed the title [analyzer/ffi] Analyzer throws .address position error for native leaf call if called with undefined getters [docs/ffi] .address position should mention .cast() Sep 2, 2024
@dcharkes dcharkes added area-documentation Prefer using 'type-documentation' and a specific area label. and removed area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Sep 2, 2024
@codesculpture
Copy link
Contributor Author

codesculpture commented Sep 2, 2024

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
Copy link
Contributor

dcharkes commented Sep 2, 2024

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:

  • Use git blame on the relevant lines of code in the analyzer and CFE (you can see this on GitHub).
  • Go the commits, look at the PR/CL link in the commit.
  • Read the documentation on those PRs/CLs.
  • Read the GitHub issues linked from those PRs/CLs.

@codesculpture
Copy link
Contributor Author

codesculpture commented Sep 3, 2024

@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
Copy link
Contributor

dcharkes commented Sep 4, 2024

@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
Copy link
Contributor Author

codesculpture commented Sep 4, 2024

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
Copy link
Contributor

dcharkes commented Sep 5, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@codesculpture
Copy link
Contributor Author

copybara-service bot pushed a commit that referenced this issue Sep 13, 2024
… ffi

Currently, we just "removed" the actual expression by transforming to
`InvalidExpression('Invalid Type')` if a expression is Invalid. But this  could cascade `.address` position errors (i.e transformer no longer able to walk `.address.abc` if the expression is just replaced with empty expression). So unwrapping actual expression from `InvalidExpression` and trying to transforming that actual expression through a recursive call.

TEST=tests/ffi/static_checks/address_position_cascade_test.dart
Bug: #56613
Change-Id: Ib1ba1d5021797b645c29ac296752d844d0935964
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/384080
Reviewed-by: Daco Harkes <[email protected]>
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Slava Egorov <[email protected]>
@codesculpture
Copy link
Contributor Author

Hi @dcharkes, Any update on updating docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-documentation Prefer using 'type-documentation' and a specific area label. library-ffi P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants