dart-lang / language

Design of the Dart language
Other
2.61k stars 200 forks source link

Add a compile-time constant for platform #1889

Open alexeyinkin opened 2 years ago

alexeyinkin commented 2 years ago

Currently there are few ways to check for the platform. They are mostly runtime and have downsides. Ways to check for different platforms are inconsistent.

A cleaner and faster solution would be to have one constant for a platform so it could be used in a switch so that a compiler could drop unneeded branches for every platform.

Dart uses different compilers for different platforms, doesn't it? So it should be possible?

mateusfccp commented 2 years ago

@alexeyinkin I think it could be possible. Currently, the better way to do this is by using Dart defines, which are resolved at compile-time, but I agree that they are a little more error-prone.

This question seems to not belong to this repo, that is concerned with language evolution, but to dart-lang/sdk instead.

mit-mit commented 2 years ago

cc @lrhn

mraleph commented 2 years ago

To summarise some discussions we had between @mit-mit @lrhn and me.

One of the approaches we could adopt now is to lean on environment constants more and change the code that does system detection in a way that allows its runtime behaviour to be overriden / short-circuited though environment constants.

This means we would rewrite the code that does this:

class Platform {
  // Can't be analyzed by the compiler.
  external static bool get isLinux;  
}

into something like this

class Platform {
  @pragma('vm:external-name', 'f')
  external static bool _isLinuxImpl();

  static const _isLinuxFromOverride =
      String.fromEnvironment('os.override') == 'linux';

  static bool get isLinux => const bool.hasEnvironment('os.override')
      ? _isLinuxFromOverride
      : _isLinuxImpl();
}

This way build system could set os.override define when building for a specific platform and the compilation toolchain would handle the rest. Though we might want to beef up our simple unreachable code elimination pass which we run prior to TFA to handle if (Platform.isLinux) a bit better - it can fold it to be get isLinux => false if -Dos.override=ios is specified but would not remove then-branch of the if. (and TFA itself is not flow sensitive - and thus will consider both branches of if to be executed).

@lrhn is changing os_detect package to incorporate this pattern https://github.com/dart-lang/os_detect/pull/15

We have also considered to apply the same pattern to dart:io Platform class - but @mkustermann points out that this has some unfortunate implications: currently our platform binary (core libraries compiled to Kernel AST) are "platform independent" and this approach only works if we make them platform dependent, so we might want to find some other approach that would work for there.

In the end we might need to simply special case Platform.isX through a Kernel transformation pass (something that we have planned before).

mkustermann commented 2 years ago

May I ask why we make this an optional override? It means sometimes kernel ASTs know their target OS/Arch/... and sometimes not. This puts us in an awkward spot where Kernel ASTs are seemingly target independent but not always - which seems a little fishy to me.

If we made it mandatory to know the target OS/Arch/... when producing Kernel ASTs we could also remove some other logic (e.g. FFI currently produces code along the lines of const [2, 4, 6](_abi()) - where _abi() is something the backend compiler consuming the AST will fold into a constant, if the Kernel AST would always know it's OS/Arch/... that could be avoided)

mkustermann commented 2 years ago

Are we only looking into this for the purpose of reducing AOT code size? If so, then it seems to me the simplest solution would be to provide OS/Arch/... once we run global AOT transformations on the kernel. We wouldn't need to use any magic string override constants to do that.

mit-mit commented 2 years ago

The goal is to reduce the code size of code that is compiled for production, which we simplified to mean AOT (on Native platforms) and dart2js (on Web platforms).

mit-mit commented 2 years ago

Related SDK issue: https://github.com/dart-lang/sdk/issues/31969

dcharkes commented 2 years ago

(e.g. FFI currently produces code along the lines of const [2, 4, 6](_abi()) - where _abi() is something the backend compiler consuming the AST will fold into a constant, if the Kernel AST would always know it's OS/Arch/... that could be avoided)

Discussion with @mkustermann and @mraleph earlier this week: With the addition of AbiSpecificIntegers (#42563), we also have our checking of whether the mapping contains the ABI that the VM is currently running on at kernel loading time, after the CFE.

nate-thegrate commented 3 weeks ago

We have better options now than when this issue was posted.

flutter/lib/src/foundation/constants.dart

// Added July 2019
const bool kIsWeb = identical(0, 0.0);

// October 2022 onward
const bool kIsWeb = bool.fromEnvironment('dart.library.js_util');


Platform.isAndroid … is not supported in web

Flutter now has defaultTargetPlatform, which works anywhere.


Perhaps it's time to close the issue?

h3x4d3c1m4l commented 3 weeks ago

That's not a const @nate-thegrate. The issue specifically asks for a const solution to ensure tree shaking works properly (e.g. no Android code ends up in iOS build and vice versa). With defaultTargetPlatform being evaluated at runtime instead of compile time, tree shaking cannot work the same way.

rrousselGit commented 3 weeks ago

Apparently tree-shaking for defaultTargetPlatform was implemented even though it's not a const, through pragmas.

Still, making the variable a constant would be nice.

mkustermann commented 3 weeks ago

That's not a const @nate-thegrate. The issue specifically asks for a const solution to ensure tree shaking works properly (e.g. no Android code ends up in iOS build and vice versa). With defaultTargetPlatform being evaluated at runtime instead of compile time, tree shaking cannot work the same way.

Although it's not a const in source code, it is a constant when we compile for deployment. We added a special mechanism to make our compilers fold this away. So defaultTargetPlatform is the right recommendation at this moment:

@pragma('vm:platform-const-if', !kDebugMode)
TargetPlatform get defaultTargetPlatform => platform.defaultTargetPlatform;

Perhaps it's time to close the issue?

Not quite yet, as the defaultTargetPlatform is a flutter-specific OS detection mechanism. We are working on a package that works for flutter and non-flutter. So we'd like to keep the issue open until that lands.

h3x4d3c1m4l commented 3 weeks ago

Awesome, I did not know of this special mechanism. Thanks for the clarification @rrousselGit and @mkustermann!

rrousselGit commented 3 weeks ago

Although it's not a const in source code, it is a constant when we compile for deployment. We added a special mechanism to make our compilers fold this away. So defaultTargetPlatform is the right recommendation at this moment:

Still, there's significant value in marking it as const if possible. Because it enables user-defined flags to also be const.

It's common for folks to do something like:

bool isMyCustomFlag = something && defaultTargetPlatform == SomePlatform;

But that flag can't be const because defaultTargetPlatform isn't.

lrhn commented 3 weeks ago

A problem is that the operatingSystem value cannot be const if code can be compiled in a way that can run on different platforms. Then the value isn't available until the code runs, which means that all down-stream constant computations must be delayed until runtime. Which means you can't tree-shake based on the constant value anyway.

Compiling for multiple platforms is normal for modular compilation, where different parts of the program are compiled separately, then combined at the end, and sometimes a program can be compiled to a reusable intermediate form. (Kernel snapshot? I don't remember the name.)

We can probably tell early on whether it's native compilation or not, with "not" currently meaning "web". With dart2wasm, I'm not sure there'll only be one "not native" in the future.

That said, we allow String.fromEnvironment to be supplied at link-time for those modules, so if each compiler introduced a dart.platform.name constant no later than at link-time, then we might be able to use that. Or have dart.tool.vm set when the VM is compiling, dart.tool.dart2js when dart2js is compiling, etc., and then you can try to infer something from that.

rrousselGit commented 3 weeks ago

That's what I was hoping personally: To have the Flutter tooling inject a --dart-define for the platform variable based in compilation target. After-all, Platform code is Flutter-specific at the moment.

jakemac53 commented 3 weeks ago

Compiling for multiple platforms is normal for modular compilation, where different parts of the program are compiled separately, then combined at the end, and sometimes a program can be compiled to a reusable intermediate form. (Kernel snapshot? I don't remember the name.)

Fwiw, everywhere modular compilation is supported by us we do not share kernel files across backend targets (compilers) anyways. So we can evaluate constants prior to linking, and some backends do evaluate constants prior to linking. One downside of course is we have to throw away the world when any global constants like this change.

Clarification: by "modular compilation" I mean totally separate compilation to separate dill files which are later passed together to a backend (compiler).