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

Add a compile-time constant for platform #1889

Open
alexeyinkin opened this issue Oct 7, 2021 · 17 comments
Open

Add a compile-time constant for platform #1889

alexeyinkin opened this issue Oct 7, 2021 · 17 comments
Labels
enhanced-const Requests or proposals about enhanced constant expressions feature Proposed language feature that solves one or more problems

Comments

@alexeyinkin
Copy link

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.

  • kIsWeb of flutter/foundation is the most accepted way to check if the code is built for web. It translates to const bool kIsWeb = identical(0, 0.0); which is not future-proof. Who knows what other platforms Dart may support in the future that do not tell int from double?
  • For other platforms we have fields like Platform.isAndroid of dart:io which is not supported in web. These translate to static final bool isAndroid = (_operatingSystem == "android"); They take runtime comparison of strings which is not a first idea of efficiency when something is supposed to never change.

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?

@alexeyinkin alexeyinkin added the feature Proposed language feature that solves one or more problems label Oct 7, 2021
@mateusfccp
Copy link
Contributor

mateusfccp commented Oct 7, 2021

@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
Copy link
Member

mit-mit commented Oct 7, 2021

cc @lrhn

@mraleph
Copy link
Member

mraleph commented Oct 8, 2021

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 dart-archive/os_detect#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.

  • we could consider leaving some constants unevaluated in the platform (and evaluate them later, but still in the CFE/Kernel compilation stage).

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

@mkustermann
Copy link
Member

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
Copy link
Member

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
Copy link
Member

mit-mit commented Oct 11, 2021

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
Copy link
Member

mit-mit commented Nov 23, 2021

Related SDK issue: dart-lang/sdk#31969

@eernstg eernstg added the enhanced-const Requests or proposals about enhanced constant expressions label Dec 1, 2021
@dcharkes
Copy link
Contributor

(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
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

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

@rrousselGit
Copy link

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
Copy link
Member

lrhn commented Jun 14, 2024

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
Copy link

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
Copy link
Contributor

jakemac53 commented Jun 14, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhanced-const Requests or proposals about enhanced constant expressions feature Proposed language feature that solves one or more problems
Projects
None yet
Development

No branches or pull requests