-
Notifications
You must be signed in to change notification settings - Fork 81
[ffigen] Fix some subtle AST iteration order bugs #2669
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
base: main
Are you sure you want to change the base?
Conversation
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with
API leaks
|
Package | Leaked API symbol | Leaking sources |
---|---|---|
objective_c | _FinalizablePointer | internal.dart::_ObjCReference::new::_finalizable |
This check can be disabled by tagging the PR with skip-leaking-check
.
License Headers ⚠️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files |
---|
pkgs/objective_c/lib/src/ns_input_stream.dart |
All source files should start with a license header.
This check can be disabled by tagging the PR with skip-license-check
.
This might also be a problem for jnigen. I need to investigate! |
Some of the transformation passes are built under the assumption that supertypes are 100% done processing before child types start processing. To ensure this ordering, they would visit the current node's supertype, then do the current node's processing, then visit the current node's children.
This usually worked, but there are APIs where this approach didn't work:
If the visitor happens to begin its traversal at
B
, then the iteration looks like(B (A (C) A) B)
, because it followsA
's methodm
toC
beforeB
is done processing. WhenC
tries to iterate to its super type, the visitor logic returns immediately, sinceB
is already being visited, soC
begins its processing beforeB
is finished.This approach would work if the AST was a DAG, but if you include full transitive iteration over all the class's methods, then it's no longer a DAG. So to fix the bug, I've added an option to the
visitChildren
method that only iterates the DAGy parts of the ObjC type graph.Any visitor that cares about this strict ordering of supertypes before subtypes should set
typeGraphOnly: true
when iterating over interfaces, protocols, and categories. This was a straightforward change inCopyMethodsFromSuperTypesVisitation
, andFixOverriddenMethodsVisitation
.But when I tried to apply it in
FindSymbolsVisitation
, it was more complicated. The main thingtypeGraphOnly: true
does is stop iterating over the methods, butFindSymbolsVisitation
needs to iterate over the methods. I found it was easier to split this visitor into two, one that creates the scopes, and one that adds symbols to scopes. ButCreateScopesVisitation
still needs to visit the methods to create their scopes, so this visitor is run twice, once withtypeGraphOnly: true
to set the scope parenting correctly, and once withtypeGraphOnly: false
to hit the AST nodes that were missed by the first pass.The other significant change is that
ObjCMethod
's local scope is now parented to the super-most type that has the method, instead of sitting in the global scope. This causes some annoying naming decisions (eg someNSData
methods that take alength
param now call itlength$1
to avoid colliding with thelength
method). But these will be fixed by #2054Fixes #2656