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

probobuf aware treeshaker incorrectly throws away empty libraries #52768

Open
mraleph opened this issue Jun 23, 2023 · 4 comments
Open

probobuf aware treeshaker incorrectly throws away empty libraries #52768

mraleph opened this issue Jun 23, 2023 · 4 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P3 A lower priority bug or feature request triaged Issue has been triaged by sub team

Comments

@mraleph
Copy link
Member

mraleph commented Jun 23, 2023

Protobuf aware tree-shakingb binary ('pkg/vm/bin/protobuf_aware_treeshaker.dart') does the following:

  final printer = BinaryPrinter(sink, libraryFilter: (lib) {
    if (removeCoreLibs && isCoreLibrary(lib)) return false;
    if (isLibEmpty(lib)) return false;
    return true;
  }, includeSources: !removeSource);

// ...

bool isLibEmpty(Library lib) {
  return lib.classes.isEmpty &&
      lib.procedures.isEmpty &&
      lib.fields.isEmpty &&
      lib.typedefs.isEmpty;
}

However even if the library is empty that does not mean it can be safely dropped because there might be constants which refer to it, most specifically private symbols, so if you pass the following code through protobuf aware treeshaking

// main.dart

import 'lib.dart';

void main() {
  print(#sym)
}

// lib.dart

const sym = #_privateSym;

You get incorrect kernel out which contains a dangling reference to lib.dart library.

@mraleph mraleph added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Jun 23, 2023
@mraleph
Copy link
Member Author

mraleph commented Jun 23, 2023

@alexmarkov
Copy link
Contributor

Is there a reason to maintain protobuf_aware_treeshaker? It's an outdated and untested wrapper over TFA and the same can done using gen_kernel.

@alexmarkov alexmarkov removed their assignment Jun 23, 2023
@mraleph
Copy link
Member Author

mraleph commented Jun 27, 2023

@alexmarkov I think we can remove the binary if we migrate internal users away from it.

@a-siva a-siva added triaged Issue has been triaged by sub team P3 A lower priority bug or feature request labels Nov 30, 2023
@a-siva
Copy link
Contributor

a-siva commented Nov 30, 2023

Maybe we should take this up with the Dart/Flutter in google3 team to figure out if it is possible to remove this tool and replace with gen_kernel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P3 A lower priority bug or feature request triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

3 participants