Skip to content

Removed unsound null safety options from classes ModuleMetadata and MetadataProvider #2510

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

Merged
merged 15 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

- Replace deprecated JS code `this.__proto__` with `Object.getPrototypeOf(this)`. - [#2500](https://github.com/dart-lang/webdev/pull/2500)
- Migrate injected client code to `package:web`. - [#2491](https://github.com/dart-lang/webdev/pull/2491)
- Removed unsound null safety options from classes ModuleMetadata, MetadataProvider & Metadata_test. - [#2427](https://github.com/dart-lang/webdev/issues/2427)
- Deprecated MetadataProvider's, CompilerOptions', SdkConfiguration's & SdkLayout's soundNullSafety. - [#2427](https://github.com/dart-lang/webdev/issues/2427)

## 24.1.0

Expand Down
1 change: 1 addition & 0 deletions dwds/lib/src/debugging/metadata/provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class MetadataProvider {
/// A sound null safety mode for the whole app.
///
/// All libraries have to agree on null safety mode.
@Deprecated('Only sound null safety is supported as of Dart 3.0')
Future<bool> get soundNullSafety async {
await _initialize();
return true;
Expand Down
8 changes: 1 addition & 7 deletions dwds/lib/src/services/chrome_proxy_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -205,16 +205,10 @@ class ChromeProxyService implements VmServiceInterface {
final canaryFeatures = loadStrategy.buildSettings.canaryFeatures;
final experiments = loadStrategy.buildSettings.experiments;

// TODO(annagrin): Read null safety setting from the build settings.
final metadataProvider = loadStrategy.metadataProviderFor(entrypoint);
final soundNullSafety = await metadataProvider.soundNullSafety;

_logger.info('Initializing expression compiler for $entrypoint '
'with sound null safety: $soundNullSafety');
_logger.info('Initializing expression compiler for $entrypoint');

final compilerOptions = CompilerOptions(
moduleFormat: ModuleFormat.values.byName(moduleFormat),
soundNullSafety: soundNullSafety,
canaryFeatures: canaryFeatures,
experiments: experiments,
);
Expand Down
5 changes: 4 additions & 1 deletion dwds/lib/src/services/expression_compiler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@
/// Options passed to DDC and the expression compiler.
class CompilerOptions {
final ModuleFormat moduleFormat;

@Deprecated('Only sound null safety is supported as of Dart 3.0')
final bool soundNullSafety;

final bool canaryFeatures;
final List<String> experiments;

CompilerOptions({
required this.moduleFormat,
required this.soundNullSafety,
this.soundNullSafety = true,
required this.canaryFeatures,
required this.experiments,
});
Expand Down
17 changes: 3 additions & 14 deletions dwds/lib/src/services/expression_compiler_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ class _Compiler {
/// expression compilation (summaries, libraries spec, compiler worker
/// snapshot).
///
/// [soundNullSafety] indicates if the compiler should support sound
/// null safety.
///
/// Performs handshake with the isolate running expression compiler
/// worker to establish communication via send/receive ports, returns
/// the service after the communication is established.
Expand All @@ -69,16 +66,10 @@ class _Compiler {
bool verbose,
) async {
sdkConfiguration.validateSdkDir();
if (compilerOptions.soundNullSafety) {
sdkConfiguration.validateSoundSummaries();
} else {
sdkConfiguration.validateWeakSummaries();
}
sdkConfiguration.validateSummaries();

final workerUri = sdkConfiguration.compilerWorkerUri!;
final sdkSummaryUri = compilerOptions.soundNullSafety
? sdkConfiguration.soundSdkSummaryUri!
: sdkConfiguration.weakSdkSummaryUri!;
final sdkSummaryUri = sdkConfiguration.sdkSummaryUri!;

final args = [
'--experimental-expression-compiler',
Expand All @@ -91,9 +82,7 @@ class _Compiler {
'--module-format',
compilerOptions.moduleFormat.name,
if (verbose) '--verbose',
compilerOptions.soundNullSafety
? '--sound-null-safety'
: '--no-sound-null-safety',
'--sound-null-safety',
for (final experiment in compilerOptions.experiments)
'--enable-experiment=$experiment',
if (compilerOptions.canaryFeatures) '--canary',
Expand Down
58 changes: 22 additions & 36 deletions dwds/lib/src/utilities/sdk_configuration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,25 +42,24 @@ class SdkLayout {
SdkLayout.createDefault(defaultSdkDirectory);

final String sdkDirectory;
final String summaryPath;
final String dartdevcSnapshotPath;

@Deprecated('Only sound null safety is supported as of Dart 3.0')
final String soundSummaryPath;

@Deprecated('Only sound null safety is supported as of Dart 3.0')
final String weakSummaryPath;
final String dartdevcSnapshotPath;

SdkLayout.createDefault(String sdkDirectory)
: this(
sdkDirectory: sdkDirectory,
soundSummaryPath: p.join(
summaryPath: p.join(
sdkDirectory,
'lib',
'_internal',
'ddc_outline.dill',
),
weakSummaryPath: p.join(
sdkDirectory,
'lib',
'_internal',
'ddc_outline_unsound.dill',
),
dartdevcSnapshotPath: p.join(
sdkDirectory,
'bin',
Expand All @@ -71,8 +70,9 @@ class SdkLayout {

const SdkLayout({
required this.sdkDirectory,
required this.soundSummaryPath,
required this.weakSummaryPath,
required this.summaryPath,
this.soundSummaryPath = '',
this.weakSummaryPath = '',
required this.dartdevcSnapshotPath,
});
}
Expand All @@ -87,12 +87,18 @@ class SdkConfiguration {
SdkConfiguration.fromSdkLayout(SdkLayout.defaultSdkLayout);

final String? sdkDirectory;
final String? sdkSummaryPath;
final String? compilerWorkerPath;

@Deprecated('Only sound null safety is supported as of Dart 3.0')
final String? weakSdkSummaryPath;

@Deprecated('Only sound null safety is supported as of Dart 3.0')
final String? soundSdkSummaryPath;
final String? compilerWorkerPath;

const SdkConfiguration({
this.sdkDirectory,
this.sdkSummaryPath,
this.weakSdkSummaryPath,
this.soundSdkSummaryPath,
this.compilerWorkerPath,
Expand All @@ -103,8 +109,7 @@ class SdkConfiguration {
SdkConfiguration.fromSdkLayout(SdkLayout sdkLayout)
: this(
sdkDirectory: sdkLayout.sdkDirectory,
weakSdkSummaryPath: sdkLayout.weakSummaryPath,
soundSdkSummaryPath: sdkLayout.soundSummaryPath,
sdkSummaryPath: sdkLayout.summaryPath,
compilerWorkerPath: sdkLayout.dartdevcSnapshotPath,
);

Expand All @@ -113,8 +118,7 @@ class SdkConfiguration {
path == null ? null : p.toUri(p.absolute(path));

Uri? get sdkDirectoryUri => _toUri(sdkDirectory);
Uri? get soundSdkSummaryUri => _toUri(soundSdkSummaryPath);
Uri? get weakSdkSummaryUri => _toUri(weakSdkSummaryPath);
Uri? get sdkSummaryUri => _toUri(sdkSummaryPath);

/// Note: has to be ///file: Uri to run in an isolate.
Uri? get compilerWorkerUri => _toAbsoluteUri(compilerWorkerPath);
Expand All @@ -139,28 +143,10 @@ class SdkConfiguration {
}

void validateSummaries({FileSystem fileSystem = const LocalFileSystem()}) {
validateSoundSummaries(fileSystem: fileSystem);
validateWeakSummaries(fileSystem: fileSystem);
}

void validateWeakSummaries({
FileSystem fileSystem = const LocalFileSystem(),
}) {
if (weakSdkSummaryPath == null ||
!fileSystem.file(weakSdkSummaryPath).existsSync()) {
throw InvalidSdkConfigurationException(
'Sdk summary $weakSdkSummaryPath does not exist',
);
}
}

void validateSoundSummaries({
FileSystem fileSystem = const LocalFileSystem(),
}) {
if (soundSdkSummaryPath == null ||
!fileSystem.file(soundSdkSummaryPath).existsSync()) {
if (sdkSummaryPath == null ||
!fileSystem.file(sdkSummaryPath).existsSync()) {
throw InvalidSdkConfigurationException(
'Sdk summary $soundSdkSummaryPath does not exist',
'Sdk summary $sdkSummaryPath does not exist',
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ void main() async {
testAll(
compilerOptions: CompilerOptions(
moduleFormat: ModuleFormat.ddc,
soundNullSafety: true,
canaryFeatures: true,
experiments: const <String>[],
),
Expand Down
1 change: 0 additions & 1 deletion dwds/test/expression_compiler_service_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ void main() async {
testAll(
compilerOptions: CompilerOptions(
moduleFormat: ModuleFormat.amd,
soundNullSafety: true,
canaryFeatures: false,
experiments: const <String>[],
),
Expand Down
2 changes: 1 addition & 1 deletion dwds/test/fixtures/utilities.dart
Original file line number Diff line number Diff line change
Expand Up @@ -300,5 +300,5 @@ class TestCompilerOptions extends CompilerOptions {
required super.canaryFeatures,
super.experiments = const [],
super.moduleFormat = ModuleFormat.amd,
}) : super(soundNullSafety: true);
});
}
24 changes: 0 additions & 24 deletions dwds/test/metadata_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,6 @@ import 'fixtures/fakes.dart';
import 'fixtures/utilities.dart';

const _emptySourceMetadata =
'{"version":"1.0.0","name":"web/main","closureName":"load__web__main",'
'"sourceMapUri":"foo/web/main.ddc.js.map",'
'"moduleUri":"foo/web/main.ddc.js",'
'"soundNullSafety":true,'
'"libraries":[{"name":"main",'
'"importUri":"org-dartlang-app:///web/main.dart",'
'"fileUri":"org-dartlang-app:///web/main.dart","partUris":[]}]}\n'
'// intentionally empty: package blah has no dart sources';

const _noNullSafetyMetadata =
'{"version":"1.0.0","name":"web/main","closureName":"load__web__main",'
'"sourceMapUri":"foo/web/main.ddc.js.map",'
'"moduleUri":"foo/web/main.ddc.js",'
Expand All @@ -35,7 +25,6 @@ const _fileUriMetadata =
'{"version":"1.0.0","name":"web/main","closureName":"load__web__main",'
'"sourceMapUri":"foo/web/main.ddc.js.map",'
'"moduleUri":"foo/web/main.ddc.js",'
'"soundNullSafety":true,'
'"libraries":[{"name":"main",'
'"importUri":"file:/Users/foo/blah/sample/lib/bar.dart",'
'"fileUri":"org-dartlang-app:///web/main.dart","partUris":[]}]}\n'
Expand All @@ -57,19 +46,6 @@ void main() {
await provider.libraries,
contains('org-dartlang-app:///web/main.dart'),
);
expect(await provider.soundNullSafety, isNotNull);
});

test('can parse metadata with no null safety information', () async {
final provider = MetadataProvider(
'foo.bootstrap.js',
FakeAssetReader(metadata: _noNullSafetyMetadata),
);
expect(
await provider.libraries,
contains('org-dartlang-app:///web/main.dart'),
);
expect(await provider.soundNullSafety, true);
});

test('throws on metadata with absolute import uris', () async {
Expand Down
31 changes: 11 additions & 20 deletions dwds/test/sdk_configuration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ void main() {
final defaultConfiguration =
await DefaultSdkConfigurationProvider().configuration;
defaultConfiguration.validateSdkDir();
defaultConfiguration.validateSoundSummaries();
defaultConfiguration.validateSummaries();
});

test('Cannot validate an empty configuration layout', () async {
Expand Down Expand Up @@ -55,12 +55,11 @@ void main() {
final sdkLayout = FakeSdkLayout(sdkDirectory);
final sdkConfiguration = FakeSdkLayout.createConfiguration(sdkLayout);

final soundSdkSummaryPath = sdkLayout.soundSummaryPath;
final summariesDir = p.dirname(soundSdkSummaryPath);
final sdkSummaryPath = sdkLayout.summaryPath;
final summariesDir = p.dirname(sdkSummaryPath);

Directory(summariesDir).createSync(recursive: true);
File(defaultSdkConfiguration.soundSdkSummaryPath!)
.copySync(soundSdkSummaryPath);
File(defaultSdkConfiguration.sdkSummaryPath!).copySync(sdkSummaryPath);

final compilerWorkerPath = sdkLayout.compilerWorkerPath;
final workerDir = p.dirname(compilerWorkerPath);
Expand All @@ -70,11 +69,11 @@ void main() {
.copySync(compilerWorkerPath);

expect(sdkConfiguration.sdkDirectory, equals(sdkDirectory));
expect(sdkConfiguration.soundSdkSummaryPath, equals(soundSdkSummaryPath));
expect(sdkConfiguration.sdkSummaryPath, equals(sdkSummaryPath));
expect(sdkConfiguration.compilerWorkerPath, equals(compilerWorkerPath));

sdkConfiguration.validateSdkDir();
sdkConfiguration.validateSoundSummaries();
sdkConfiguration.validateSummaries();
});

test('Cannot validate non-existing configuration layout', () async {
Expand All @@ -96,22 +95,19 @@ void main() {

final sdkLayout = SdkLayout.createDefault(sdkDirectory);
final sdkConfiguration = SdkConfiguration.fromSdkLayout(sdkLayout);
final soundSdkSummaryPath = sdkLayout.soundSummaryPath;
final weakSdkSummaryPath = sdkLayout.weakSummaryPath;
final sdkSummaryPath = sdkLayout.summaryPath;
final compilerWorkerPath = sdkLayout.dartdevcSnapshotPath;

setUp(() async {
fs = MemoryFileSystem();
await fs.directory(sdkDirectory).create(recursive: true);
await fs.file(soundSdkSummaryPath).create(recursive: true);
await fs.file(weakSdkSummaryPath).create(recursive: true);
await fs.file(sdkSummaryPath).create(recursive: true);
await fs.file(compilerWorkerPath).create(recursive: true);
});

test('Can create and validate default SDK configuration', () async {
expect(sdkConfiguration.sdkDirectory, equals(sdkDirectory));
expect(sdkConfiguration.soundSdkSummaryPath, equals(soundSdkSummaryPath));
expect(sdkConfiguration.weakSdkSummaryPath, equals(weakSdkSummaryPath));
expect(sdkConfiguration.sdkSummaryPath, equals(sdkSummaryPath));
expect(sdkConfiguration.compilerWorkerPath, equals(compilerWorkerPath));

sdkConfiguration.validateSdkDir(fileSystem: fs);
Expand All @@ -137,18 +133,13 @@ class FakeSdkLayout {
static SdkConfiguration createConfiguration(FakeSdkLayout sdkLayout) =>
SdkConfiguration(
sdkDirectory: sdkLayout.sdkDirectory,
soundSdkSummaryPath: sdkLayout.soundSummaryPath,
weakSdkSummaryPath: sdkLayout.weakSummaryPath,
sdkSummaryPath: sdkLayout.summaryPath,
compilerWorkerPath: sdkLayout.compilerWorkerPath,
);

FakeSdkLayout(this.sdkDirectory);

String get weakSummaryPath =>
p.join(sdkDirectory, 'summaries', 'unsound.dill');

String get soundSummaryPath =>
p.join(sdkDirectory, 'summaries', 'sound.dill');
String get summaryPath => p.join(sdkDirectory, 'summaries', 'sound.dill');

String get compilerWorkerPath =>
p.join(sdkDirectory, 'snapshots', 'test.snapshot');
Expand Down
Loading