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.24k stars 1.57k forks source link

probobuf aware treeshaker incorrectly throws away empty libraries #52768

Open mraleph opened 1 year ago

mraleph commented 1 year ago

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 commented 1 year ago

Also related to https://dart-review.googlesource.com/c/sdk/+/310772

alexmarkov commented 1 year ago

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.

mraleph commented 1 year ago

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

a-siva commented 11 months ago

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