[M] Change in dart/sdk[main]: [ DDC / CFE ] Add support for allowing imports of unsupported libraries
Sigmund Cherem (Gerrit)
Sigmund Cherem added 4 comments![]( "Open in Gerrit")
Just some drive-by comments to share.
: super(supported: const {'_dart2js_only', '_js_annotations'});is this change needed?
This only changes whether you can use `_js_annotations` in a conditional import, but our preference is generally not to allow using internal libraries in that context ([ref][1])
// These two native libraries have always been "supported" for DDC andI believe we don't need to add them here, as these are derived automatically from the library specification.
Suggestion: could we also lean into the library specification to declare libraries that can be imported when the flag is enabled?
The motivation is a bit nuanced. The original goal with that specification was to centralize knowledge and try to use a declarative syntax for how our targets are configured. I still imagine however that the effective result is equivalent to your current implementation, though.
The full specification is described in the specification parser here: https://github.com/dart-lang/sdk/blob/f19440c37b1433921c4bdf55cee11f9710dc816c/pkg/_fe_analyzer_shared/lib/src/util/libraries_specification.dart, and I imagine we can extend it to add a new parameter to encode the new logic without having to replicate the information in the Target implementation class.
For example, if instead of `supported: false`, we change it to:
```
io:
url: ...
support_direct_import: true
support_conditional_import: false # << equivalent to supported: false
ffi:
url: ...
support_direct_import: with_flag # << the new semantics
support_conditional_import: false # << still says false in conditional imports
```
Thoughts?
'_js_annotations',same as in the dart2js target, I don't think we should add this one. (side comment, I wish the way we did _ddc_only/_dart2js_only was to add `supported: true` to the specification to override the default, but those libraries are already a workaround because we wanted a way to distinguish tools rather than libraries in conditional imports).
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 8 comments![]( "Open in Gerrit")
Thanks so much for making the changes Ben, this looks great! I only have minor comments and suggestions below.
importability: importability,Just a comment to share with you and the CFE team (nothing to change for this CL): I believe the CFE separately has other logic to allow/disallow imports to `dart:_` libraries. Those rules include asking the target class (e.g. we allow them from tests, but not from user's code). I wonder if this new concept of importability can be generalized for that too.
List<String> get extraRequiredLibrariesPlatform => const ['dart:ffi'];I wonder if we need both this and the change above, or only one of them?
Importability.always;I wonder if the default here should be `never`. That is, the reason the first expression would be null is if we are missing an entry in the .yaml file for a specific library.
It's possible we have test coverage for it elsewhere, but if not, it may help to also add a test for trying to import a library that doesn't exist in the specification and making sure we provide the right diagnostic in that case too.
var unsupportedLibraryUris = new Set<String>();minor: consider using a List, rather than a Set, to also validate that the size of the set is 1 (only 1 diagnostic is reported). Ditto with the next test?
help: 'Whether platform specific dart:* libraries should be importable 'optional nit: consider inverting the sentence to say that the library is unsupported, rather than the runtime? Maybe "Whether to allow imports to unsupported platform libraries."
bool get isUnsupported => flags & SupportConditionalImportsFlag != 0;(not to fix on this CL) - it would be nice to make this consistent with `importability` (either combining the two, or renaming this to conditionalImportability). Maybe add a TODO or filing a bug to track this?
support_conditional_import: falseQuestion: Any reason to keep `supported` here during a transition period?
I'm mainly thinking about the rollout into google3 and flutter, where we also have library specification files that contain "imports" to this specification file. I believe the way you made the change (where `supported` is deprecated but still supported) should gracefully allow for rolling out this change without causing issues, but I'm not 100% sure. The one piece that gives me concern is where we mix of code rolled from the Flutter SDK combined with code from the Dart SDK at different versions. We kept the SDKs disjoint for mobile targets, but we do mix them a bit for web targets in g3.
Related details
- Ben Konyi
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted and added 8 comments![]( "Open in Gerrit")
Votes added by Ben Konyi
| Commit-Queue | +1 |
8 comments
: super(supported: const {'_dart2js_only', '_js_annotations'});is this change needed?
This only changes whether you can use `_js_annotations` in a conditional import, but our preference is generally not to allow using internal libraries in that context ([ref][1])
Done
List<String> get extraRequiredLibrariesPlatform => const ['dart:ffi'];I wonder if we need both this and the change above, or only one of them?
I think we need both. This property is only used when generating the `platform.dill`, which ensures that it's actually included. `extraRequiredLibraries` is used when compiling actual applications to determine which libraries should be loaded, so we probably don't want to load libraries that aren't importable.
// These two native libraries have always been "supported" for DDC andI believe we don't need to add them here, as these are derived automatically from the library specification.
Suggestion: could we also lean into the library specification to declare libraries that can be imported when the flag is enabled?
The motivation is a bit nuanced. The original goal with that specification was to centralize knowledge and try to use a declarative syntax for how our targets are configured. I still imagine however that the effective result is equivalent to your current implementation, though.
The full specification is described in the specification parser here: https://github.com/dart-lang/sdk/blob/f19440c37b1433921c4bdf55cee11f9710dc816c/pkg/_fe_analyzer_shared/lib/src/util/libraries_specification.dart, and I imagine we can extend it to add a new parameter to encode the new logic without having to replicate the information in the Target implementation class.
For example, if instead of `supported: false`, we change it to:
```
io:
url: ...
support_direct_import: true
support_conditional_import: false # << equivalent to supported: falseffi:
url: ...
support_direct_import: with_flag # << the new semantics
support_conditional_import: false # << still says false in conditional imports
```Thoughts?
Done
same as in the dart2js target, I don't think we should add this one. (side comment, I wish the way we did _ddc_only/_dart2js_only was to add `supported: true` to the specification to override the default, but those libraries are already a workaround because we wanted a way to distinguish tools rather than libraries in conditional imports).
Done
Importability.always;I wonder if the default here should be `never`. That is, the reason the first expression would be null is if we are missing an entry in the .yaml file for a specific library.
It's possible we have test coverage for it elsewhere, but if not, it may help to also add a test for trying to import a library that doesn't exist in the specification and making sure we provide the right diagnostic in that case too.
Yeah, I think this can be `never`. We should be checking that this is a `dart:*` URI before trying to call this function, so we can default to `always` in the situation where we're not looking at a core library where this is called.
var unsupportedLibraryUris = new Set<String>();minor: consider using a List, rather than a Set, to also validate that the size of the set is 1 (only 1 diagnostic is reported). Ditto with the next test?
Done
bool get isUnsupported => flags & SupportConditionalImportsFlag != 0;(not to fix on this CL) - it would be nice to make this consistent with `importability` (either combining the two, or renaming this to conditionalImportability). Maybe add a TODO or filing a bug to track this?
Not sure how I missed the part that said not to fix on this CL, but I fixed it on this CL... I don't think it adds too much, but I can revert if needed.
support_conditional_import: falseQuestion: Any reason to keep `supported` here during a transition period?
I'm mainly thinking about the rollout into google3 and flutter, where we also have library specification files that contain "imports" to this specification file. I believe the way you made the change (where `supported` is deprecated but still supported) should gracefully allow for rolling out this change without causing issues, but I'm not 100% sure. The one piece that gives me concern is where we mix of code rolled from the Flutter SDK combined with code from the Dart SDK at different versions. We kept the SDKs disjoint for mobile targets, but we do mix them a bit for web targets in g3.
I was keeping it around with the assumption that there were uses of `supported` in Flutter, but it doesn't look like there is. I'm going to try removing it and see what happens.
Related details
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 5 comments![]( "Open in Gerrit")
Nice! LGTM, thanks for making all the changes.
final List<Library> libraries = loader.libraries;Nice that you moved this here - seems like we can keep the logic within the CFE for now and not expose ways to override it on targets yet if we don't need to.
bool get isUnsupported => flags & SupportConditionalImportsFlag != 0;Ben Konyi(not to fix on this CL) - it would be nice to make this consistent with `importability` (either combining the two, or renaming this to conditionalImportability). Maybe add a TODO or filing a bug to track this?
Not sure how I missed the part that said not to fix on this CL, but I fixed it on this CL... I don't think it adds too much, but I can revert if needed.
Thanks, looks good to me :)
support_conditional_import: falseBen KonyiQuestion: Any reason to keep `supported` here during a transition period?
I'm mainly thinking about the rollout into google3 and flutter, where we also have library specification files that contain "imports" to this specification file. I believe the way you made the change (where `supported` is deprecated but still supported) should gracefully allow for rolling out this change without causing issues, but I'm not 100% sure. The one piece that gives me concern is where we mix of code rolled from the Flutter SDK combined with code from the Dart SDK at different versions. We kept the SDKs disjoint for mobile targets, but we do mix them a bit for web targets in g3.
I was keeping it around with the assumption that there were uses of `supported` in Flutter, but it doesn't look like there is. I'm going to try removing it and see what happens.
Oh, sorry for the confusion and ambiguity on my initial message - I meant actually a bit the opposite: instead of just keep `supported` in the implementation of the spec format, I was wondering if we should also keep `supported` in the .yaml/.json file for a while longer and wait to remove it after we do a roll and we see that everything continues to work.
That said - I have no strong opinions here. I'm happy with any approach. I was just looking to reduce friction/toil in the rollout process.
Related details
- Ben Konyi
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi added 1 comment![]( "Open in Gerrit")
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi added 1 comment![]( "Open in Gerrit")
Okay, I think this is ready for an actual review now. PTAL!
Related details
- Jens Johansen
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jens Johansen (Gerrit)
Jens Johansen added 5 comments![]( "Open in Gerrit")
I don't have any objections, but have left a few comments. (would also like some more unit tests to get rid of some of the coverage ignore comments)
I will refer to Johnni though as he knows more about the unsupported/supported stuff.
final Object? supportConditionalImport =
data['support_conditional_import'];
if (supportConditionalImport != null &&
supportConditionalImport is! bool) {
_reportError(
messagePropertyIsNotABool(
'support_conditional_import',
supportConditionalImport,
),
);
}Maybe
```
final Object supportConditionalImport =
data['support_conditional_import'] ?? true;
if (supportConditionalImport is! bool) {
_reportError(
messagePropertyIsNotABool(
'support_conditional_import',
supportConditionalImport,
),
);
}
```
would read better (and avoid the `?? true` below)
/// Specifies if the library is importable on the target platform.`if` doesn't seem to capture a three-valued enum. Also below.
// TODO: is there a better way to get this offset?
dependency.fileOffset + 'import '.length,I do think we should do this. Both `import "foo.dart"` and
```
import
"foo.dart"
```
are perfectly legal.
conditionalImportSupported:
origin?.conditionalImportSupported ??
isDartLib && target.uriTranslator.isLibrarySupported(importUri.path),Hasn't the logic been inverted here?
Before in the `??` case on a non-dart-import it would be `isUnsupported: false`, now the same situation becomes `conditionalImportSupported: false`. In other places a default of `true` has become `false` (e.g. below).
(Or is it a bug-fix, because non-dart libraries should never be supported? And maybe more a no-op because it's only used for dart libraries anyway?)
Related details
- Ben Konyi
- Johnni Winther
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted and added 4 comments![]( "Open in Gerrit")
Votes added by Ben Konyi
| Commit-Queue | +1 |
4 comments
final Object? supportConditionalImport =
data['support_conditional_import'];
if (supportConditionalImport != null &&
supportConditionalImport is! bool) {
_reportError(
messagePropertyIsNotABool(
'support_conditional_import',
supportConditionalImport,
),
);
}Maybe
```
final Object supportConditionalImport =
data['support_conditional_import'] ?? true;
if (supportConditionalImport is! bool) {
_reportError(
messagePropertyIsNotABool(
'support_conditional_import',
supportConditionalImport,
),
);
}
```would read better (and avoid the `?? true` below)
Done
/// Specifies if the library is importable on the target platform.`if` doesn't seem to capture a three-valued enum. Also below.
Done
// TODO: is there a better way to get this offset?
dependency.fileOffset + 'import '.length,I do think we should do this. Both `import "foo.dart"` and
```
import
"foo.dart"
```are perfectly legal.
Yeah, this felt weird to me. I've inverted the logic here to look at accessors of libraries rather than imports / exports within a library to get access to the correct offsets.
conditionalImportSupported:
origin?.conditionalImportSupported ??
isDartLib && target.uriTranslator.isLibrarySupported(importUri.path),Hasn't the logic been inverted here?
Before in the `??` case on a non-dart-import it would be `isUnsupported: false`, now the same situation becomes `conditionalImportSupported: false`. In other places a default of `true` has become `false` (e.g. below).(Or is it a bug-fix, because non-dart libraries should never be supported? And maybe more a no-op because it's only used for dart libraries anyway?)
Yeah, non-dart libraries should never be supported WRT conditional imports and `isLibrarySupported` should only be used to determine if a dart: library is supported.
Related details
- Jens Johansen
- Johnni Winther
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 1 comment![]( "Open in Gerrit")
} else if (arg == '--include-unsupported-platform-library-stubs') {
includeUnsupportedPlatformLibraryStubs = true;Summary from our discussion:
- This flag is only propagated to the creation of the `Target`.
- The widget previews rely on the frontend server to create the `Target` instance.
- We can probably remove the command line flag from DDC for now and add it if we have a direct use when compiling from the command line invocation.
Related details
- Ben Konyi
- Jens Johansen
- Johnni Winther
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther voted Code-Review+1![]( "Open in Gerrit")
- Ben Konyi
- Jens Johansen
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ben Konyi
| Commit-Queue | +1 |
1 comment
} else if (arg == '--include-unsupported-platform-library-stubs') {
includeUnsupportedPlatformLibraryStubs = true;Summary from our discussion:
- This flag is only propagated to the creation of the `Target`.
- The widget previews rely on the frontend server to create the `Target` instance.
- We can probably remove the command line flag from DDC for now and add it if we have a direct use when compiling from the command line invocation.
- Jens Johansen
- Johnni Winther
- Nicholas Shahan
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Jens Johansen
- Johnni Winther
- Nicholas Shahan
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther voted Code-Review+1![]( "Open in Gerrit")
- Ben Konyi
- Jens Johansen
- Nicholas Shahan
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 2 comments![]( "Open in Gerrit")
I really hope that in the future we can find a more wholistic approach for handling platform specific libraries. This does the job for this targeted use case but like we've discussed I don't think it offers a good solution for end users and the package ecosystem if we truly want it to be multi-platform. Thanks for all of the discussions around this CL.
includeUnsupportedPlatformLibraryStubs:
includeUnsupportedPlatformLibraryStubs,Wow, I was looking all over the CL for this code. I didn't expect to find it here.
Related details
- Ben Konyi
- Jens Johansen
- Sigmund Cherem
Nicholas Shahan (Gerrit)
Nicholas Shahan voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ben Konyi
- Jens Johansen
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted Commit-Queue+2![]( "Open in Gerrit")
- Jens Johansen
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[ DDC / CFE ] Add support for allowing imports of unsupported libraries
This change adds support for allowing for imports of unsupported
platform-specific libraries when the
`--include-unsupported-platform-library-stubs` flag is provided to the
CFE.
This flag sets the `includeUnsupportedPlatformLibraryStubs` property in
`TargetFlags`, which `Target`s can use to conditionally return different
`DartLibrarySupport` objects with different supported/unsupported
library sets.
A `checkForUnsupportedDartColonImports` function has been added to
`Target` that uses the value of `dartLibrarySupport` to determine if
there's any unsupported library imports. This function is called after
the various transformation operations provided by the `Target`
implementation, meaning the import of an unsupported library specified
in `dartLibrarySupport` will now result in a compilation error (this
includes `dart:mirrors` imports for VM targets when mirrors are
disabled, which was previously handled by the VM itself).
Related to https://github.com/dart-lang/sdk/issues/62125
TEST=Tests added / modified
- M pkg/_fe_analyzer_shared/lib/src/util/libraries_specification.dart
- M pkg/_fe_analyzer_shared/test/util/libraries_specification_test.dart
- M pkg/dev_compiler/lib/src/kernel/target.dart
- M pkg/dev_compiler/test/expression_compiler/expression_compiler_test.dart
- M pkg/front_end/lib/src/base/incremental_compiler.dart
- M pkg/front_end/lib/src/base/uri_translator.dart
- M pkg/front_end/lib/src/builder/compilation_unit.dart
- M pkg/front_end/lib/src/builder/library_builder.dart
- M pkg/front_end/lib/src/codes/cfe_codes_generated.dart
- M pkg/front_end/lib/src/dill/dill_library_builder.dart
- M pkg/front_end/lib/src/kernel/benchmarker.dart
- M pkg/front_end/lib/src/kernel/constant_evaluator.dart
- M pkg/front_end/lib/src/kernel/kernel_target.dart
- M pkg/front_end/lib/src/source/source_compilation_unit.dart
- M pkg/front_end/lib/src/source/source_library_builder.dart
- M pkg/front_end/lib/src/source/source_loader.dart
- M pkg/front_end/messages.yaml
- M pkg/front_end/test/generator_to_string_test.dart
- M pkg/front_end/test/spell_checking_list_code.txt
- M pkg/front_end/test/spell_checking_list_common.txt
- M pkg/front_end/test/spell_checking_list_messages.txt
- M pkg/front_end/test/static_types/cfe_allowed.json
- M pkg/front_end/test/types/cfe_types_test.dart
- M pkg/front_end/test/types/kernel_type_parser_test.dart
- A pkg/front_end/test/unsupported_platform_dart_library_import_test.dart
- M pkg/front_end/testcases/general/patch_internal/main.dart.strong.expect
- M pkg/front_end/testcases/general/patch_internal/main.dart.strong.modular.expect
- M pkg/front_end/testcases/general/patch_internal/main.dart.strong.outline.expect
- M pkg/front_end/testcases/general/patch_internal/main.dart.strong.transformed.expect
- A pkg/front_end/testcases/general/supported_libraries/import_unsupported.not.importable_lib.dart
- M pkg/front_end/testcases/general/supported_libraries/libraries.json
- M pkg/front_end/testcases/general/supported_libraries/main.dart
- M pkg/front_end/testcases/general/supported_libraries/main.dart.strong.expect
- M pkg/front_end/testcases/general/supported_libraries/main.dart.strong.modular.expect
- M pkg/front_end/testcases/general/supported_libraries/main.dart.strong.outline.expect
- M pkg/front_end/testcases/general/supported_libraries/main.dart.strong.transformed.expect
- M pkg/front_end/testcases/general/supported_libraries/main.dart.textual_outline.expect
- M pkg/front_end/testcases/general/supported_libraries/main.dart.textual_outline_modelled.expect
- A pkg/front_end/testcases/general/supported_libraries/unsupported.not.importable_lib.dart
- M pkg/front_end/tool/dart_doctest_impl.dart
- M pkg/frontend_server/lib/frontend_server.dart
- M pkg/kernel/lib/ast.dart
- M pkg/kernel/lib/src/ast/libraries.dart
- M pkg/kernel/lib/target/targets.dart
- M pkg/kernel/lib/testing/type_parser_environment.dart
- M pkg/kernel/lib/text/ast_to_text.dart
- M pkg/vm/lib/kernel_front_end.dart
- M pkg/vm/testcases/transformations/mixin_deduplication/generic.dart.expect
- M pkg/vm/testcases/transformations/mixin_deduplication/generic_recursive_bounds.dart.expect
- M pkg/vm/testcases/transformations/mixin_deduplication/multiple_libraries_main.dart.expect
- M pkg/vm/testcases/transformations/mixin_deduplication/non_generic.dart.expect
- M sdk/lib/libraries.json
- M sdk/lib/libraries.yaml
- M tests/co19/co19-kernel.status
- M tests/lib/js/static_interop_test/disallowed_interop_libraries_test.dart
Code-Review: +1 by Nicholas Shahan, +1 by Johnni Winther
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |