Skip to content
This repository was archived by the owner on Oct 18, 2024. It is now read-only.

Conversation

lrhn
Copy link
Contributor

@lrhn lrhn commented Jul 13, 2023

By using the VM's @pragma("vm:platform-const") annotation, and Platform members annotated with it, ensure that the isLinux and operatingSystem values are also known at compile-time, even if they cannot be annotated as platform-constants.

This is done by having a class per known operating system, and making sure to only create instances of those classes guarded by platform-constants (or real constants), so that if nobody goes out of their way to create a custom instance, which they should only do for testing, all but one of those platform-specific classes can be tree-shaken.

At that point, checks like is MacOS (one of the types) can be statically determined to be false if MacOS was tree-shaken, and .id can be statically resolved to a single final variable with a known value, which enables tests based on those to tree-shake unreachable branches.

(Also cleaned up the lints, removed the bad ones, added more good ones from our working list for team-lints.)

Thanks to @jonasfj for the inspired idea to use class-tree-shaking to propagate "platform const" to non-constant code.

@lrhn lrhn requested a review from natebosch July 13, 2023 14:07
By using the VM's `@pragma("vm:platform-const")` annotation,
and `Platform` members annotated with it, ensure that the
`isLinux` and `operatingSystem` values are also known at compile-time,
even if they cannot be annotated as platform-constants.

This is done by having a class per known operating system,
and making sure to only create instances of those classes
guarded by platform-constants (or real constants), so that
if nobody goes out of their way to create a custom instance,
which they should only do for testing, all but one of those
platform-specific classes can be tree-shaken.

At that point, checks like `is MacOS` (one of the types)
can be statically determined to be false if `MacOS` was tree-shaken,
and `.id` can be statically resolved to a single final variable
with a known value.
@lrhn lrhn force-pushed the enable-tree-shaking branch from eb721b3 to 870e614 Compare July 13, 2023 14:09
@lrhn
Copy link
Contributor Author

lrhn commented Jul 13, 2023

More detail.

A program like:

import "package:os_detect/os_detect.dart" as Platform;
void main() {
  if (Platform.isLinux) { 
    print("Is Linux");
  } else {
    print("SOMETHING ELSE");
  }
  if (Platform.operatingSystem == "linux") {
    print("Is Linux");
  } else {
    print("SOMETHING ELSE");
  }
}

compiled as dart compile exe --target-os=linux example.dart will generate an executeable so that

strings example.exe | grep "SOMETHING ELSE"

has no matches. The branches are tree-shaken.

And it still supports overriding the OS for testing:

import "package:os_detect/os_detect.dart" as Platform;
import "package:os_detect/override.dart" as Platform;
void main() {
  test(); // prints "Is Linux" twice.
  overrideOperatingSystem(OperatingSystem("macos", "v9"), test);  // Prints "SOMETHING ELSE" twice.
}
void test() {
  if (Platform.isLinux) { 
    print("Is Linux");
  } else {
    print("SOMETHING ELSE");
  }
  if (Platform.operatingSystem == "linux") {
    print("Is Linux");
  } else {
    print("SOMETHING ELSE");
  }
}

If you use the OperatingSystem constructor, you may very well have opted out of tree-shaking entirely.
(Or you might get lucky, if you pass a constant string and the call is inlined sufficiently, so it knows you only care about Linux and MacOS.)

String get _os => Platform.operatingSystem;
String get _osVersion => Platform.operatingSystemVersion;
@pragma('vm:platform-const')
final OS? _osType = Platform.operatingSystem == 'linux'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a chain of ternary conditionals perform better than a switch?

final OS? _osType = switch (Platform.operatingSystem) {
  'linux' => const LinuxOS(),
  'macos' => const MacOS(),
  'windows' => const WindowsOS(),
  'android' => const AndroidOS(),
  'ios' => const IOS(),
  'fuchsia' => const FuchsiaOS(),
  'browser' => const BrowserOS(),
  _ => null
};

Copy link
Contributor Author

@lrhn lrhn Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it will constant fold better (or at all).

The expressions allowed by the platform-const annotation is limited, but potentially constant expressions are allowed. The VM compiler should reduce this to just one value when it knows Platform.operatingSystem. If it doesn't, the overhead is minimal, and it's executed only once.

// BSD-style license that can be found in the LICENSE file.

/// Functionality to override information about the current platform.
library pkg.os_detect.src.override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the library name? Can we use library;?

Copy link
Contributor Author

@lrhn lrhn Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can. Old habit of giving names when I write library docs. And just old code being edited.

/// for example Android (see [isAndroid]).
/// for example Android (see [isAndroid]),
/// or if the code is running inside a browser (see [isBrowser]).
@pragma('vm:prefer-inline')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that this annotation will not make guarantees, the new @pragma('vm:platform-const') will make guarantees (/cc @sstrickl )

Even if the method is inlined, it may happen in the C++ compiler backend. That has also a tree-shaker, but we want to ensure it's tree shaken before our kernel-level global analysis pass. We have this new platform-const annotation for that purpose.

If there's subsets of expression/statement language missing to make it work, @sstrickl can implement that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not expecting this getter to be platform-const, that's going to be impossible when we also have the ability to do runtime overrides.

However, if inlined twice, and all but one OSKind subclass has been tree-shaken, the compiler can still deduce that the value is always true or false. (And it works! So don't get worse at treeshaking!)

@lrhn lrhn merged commit 7f376bf into master Sep 28, 2023
@lrhn lrhn deleted the enable-tree-shaking branch September 28, 2023 06:28
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 16, 2024
…archive/os_detect#31)

* Use the VM's "platform-const" feature to achieve tree-shaking.

By using the VM's `@pragma("vm:platform-const")` annotation,
and `Platform` members annotated with it, ensure that the
`isLinux` and `operatingSystem` values are also known at compile-time,
even if they cannot be annotated as platform-constants.

This is done by having a class per known operating system,
and making sure to only create instances of those classes
guarded by platform-constants (or real constants), so that
if nobody goes out of their way to create a custom instance,
which they should only do for testing, all but one of those
platform-specific classes can be tree-shaken.

At that point, checks like `is MacOS` (one of the types)
can be statically determined to be false if `MacOS` was tree-shaken,
and `.id` can be statically resolved to a single final variable
with a known value.

* Update test matrix to start at 3.0.0

* Add example discussing tree-shaking.

* Format

* Address comments.

* Don't refer to unimported name in docs.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants