Proposal: prohibit implicit down casts from dynamic when building platform?

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.09k stars 1.56k forks source link

Proposal: prohibit implicit down casts from dynamic when building platform? #45495

Open mraleph opened 3 years ago

mraleph commented 3 years ago

You might change something accidentally to dynamic and you will not get any compilation errors - because we allow implicit down casts.

Seems dangerous for core libraries.

Another question here is: could we maybe invest in ability to run analyzer on core libraries, so that we could lint them?

/cc @leafpetersen @devoncarew @a-siva @lrhn

leafpetersen commented 3 years ago

cc @natebosch @munificent @mit-mit @lrhn who have a background project to get a set of lints in place to run on Dart owned code. A lint to disable implicit downcasts from dynamic seems like a good candidate to me. And I hope that running these on the core libraries is something we are planning for?

natebosch commented 3 years ago

A lint to disable implicit downcasts from dynamic seems like a good candidate to me.

+1 - we disable implicit casts in a bunch of our pub packages. Disabling in the SDK seems like a good idea. @sigmundch might be able to tell us if there are performance risks to adding explicit casts to the SDK in dart2js.

It's not implemented as a lint today, but I would imagine we could implement it as a lint if we need to.

And I hope that running these on the core libraries is something we are planning for?

Yes. That is one of my goals.

leafpetersen commented 3 years ago

Disabling in the SDK seems like a good idea. @sigmundch might be able to tell us if there are performance risks to adding explicit casts to the SDK in dart2js.

This will be an issue. I've tried to explicitly mark places where I'm using downcasts from dynamic for dart2js production mode performance, but I'm sure there are unmarked ones. I think we talked about replacing these with a call to an "unsafe dart2js cast" function, but I don't think we ever did that? cc @rakudrama

rakudrama commented 3 years ago

dart2js has a @pragma that can be used to ignore as casts in the function annotated. The pragma can be used to create an 'unsafe dart2js cast' function, but if the code is intended to be shared or kept in sync with DDC code, it is probably better to use the pragma directly to avoid unnecessary calls in the DDC side.

sigmundch commented 3 years ago

Indeed - I think we can workaround the downcast with either annotations or a helper unsafe cast function.

One big question is how we setup an analyzer on the sdk. Two options come to mind:

resurrect the "patch_sdk". We have used this many times in the past, for example, this is what we did to check for null-safety errors during the sdk null-safety migration. Ironically we just deleted it a couple weeks ago , but we can easily bring it back.
invest in making the analyzer understand patch files. I'm pretty sure this would take longer, but it would be pretty nice. The patch_sdk has several known limitations (the semantics are not identical to the CFE's patching mechanism, error locations are reported on the patched code and not the original code, etc).

leafpetersen commented 3 years ago

One big question is how we setup an analyzer on the sdk.

There are several existing ways to do this without it seeing the patch files, but I guess there would be a fair bit of value in analyzing the patch files as well. cc @scheglov @stereotype441 @devoncarew @bwilkerson for thoughts here.

Another possibility would be to implement the strict checking flags in the CFE as well. cc @johnniwinther @stefantsov for thoughts on that.

scheglov commented 3 years ago

Yes, I think SDK libraries can be analyzed with dartanalyzer --dart-sdk=my/sdk --options=my/analysis_options.yaml --sdk-warnings my/sdk/lib/core/. In the analysis options file you can disable analyzer/strong-mode/implicit-casts.

For me it looks better to separate patching and analysis. Patching is something specific for SDK, and is not necessary for any other code, so we don't want to support it in general purpose analysis tools. So, we need a separate tool that will apply patches, and analyze resulting SDK libraries using standard dartanalyzer.

johnniwinther commented 3 years ago

It should be no problem to support this in the CFE.

vsmenon commented 3 years ago

Do we actually need dynamic in SDK code at all? I.e., do we rely on dynamic dispatch anywhere (intentionally) or could we prohibit it?

vsmenon commented 3 years ago

( @scheglov - we did have a patch_sdk.dart that analyzer-ddc used back in the day - removed here https://github.com/dart-lang/sdk/commit/06a4b109bce970fbd98bf72774a8787124900dee )

leafpetersen commented 3 years ago

Do we actually need dynamic in SDK code at all? I.e., do we rely on dynamic dispatch anywhere (intentionally) or could we prohibit it?

We do use it, grep for "as dynamic" and you'll mostly see uses for avoiding explicit casts in dart2js, but a handful of actual invocations. I haven't looked into whether they can be eliminated.

johnniwinther commented 3 years ago

Dart2js has a test that tracks all dynamic accesses in the sdk. This is the allowed list for it https://github.com/dart-lang/sdk/blob/master/pkg/compiler/test/analyses/api_allowed.json so we currently have 254 dynamic accesses in the sdk.

srawlins commented 2 years ago

"strict-casts" is a new analysis mode, available at HEAD. See https://github.com/dart-lang/language/blob/master/resources/type-system/strict-casts.md.