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

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

Merged
merged 15 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +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)
- Deprecated MetadataProvider's, CompilerOptions', SdkConfiguration's & SdkLayout's soundNullSafety. - [#2427](https://github.com/dart-lang/webdev/issues/2427)

## 24.1.0

Expand Down
11 changes: 2 additions & 9 deletions dwds/lib/src/debugging/metadata/module_metadata.dart
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,13 @@ class ModuleMetadata {
/// Module uri
final String moduleUri;

/// True if the module corresponding to this metadata was compiled with sound
/// null safety enabled.
final bool soundNullSafety;

final Map<String, LibraryMetadata> libraries = {};

ModuleMetadata(
this.name,
this.closureName,
this.sourceMapUri,
this.moduleUri,
this.soundNullSafety, {
this.moduleUri, {
String? ver,
}) {
version = ver ?? ModuleMetadataVersion.current.version;
Expand All @@ -151,8 +146,7 @@ class ModuleMetadata {
name = _readRequiredField(json, 'name'),
closureName = _readRequiredField(json, 'closureName'),
sourceMapUri = _readRequiredField(json, 'sourceMapUri'),
moduleUri = _readRequiredField(json, 'moduleUri'),
soundNullSafety = _readOptionalField(json, 'soundNullSafety') ?? false {
moduleUri = _readRequiredField(json, 'moduleUri') {
if (!ModuleMetadataVersion.current.isCompatibleWith(version) &&
!ModuleMetadataVersion.previous.isCompatibleWith(version)) {
throw Exception('Unsupported metadata version $version. '
Expand All @@ -174,7 +168,6 @@ class ModuleMetadata {
'sourceMapUri': sourceMapUri,
'moduleUri': moduleUri,
'libraries': [for (final lib in libraries.values) lib.toJson()],
'soundNullSafety': soundNullSafety,
};
}
}
Expand Down
17 changes: 3 additions & 14 deletions dwds/lib/src/debugging/metadata/provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class MetadataProvider {
final AssetReader _assetReader;
final _logger = Logger('MetadataProvider');
final String entrypoint;
bool _soundNullSafety;
final List<String> _libraries = [];
final Map<String, String> _scriptToModule = {};
final Map<String, String> _moduleToSourceMap = {};
Expand Down Expand Up @@ -65,15 +64,15 @@ class MetadataProvider {
'dart:ui',
];

MetadataProvider(this.entrypoint, this._assetReader)
: _soundNullSafety = false;
MetadataProvider(this.entrypoint, this._assetReader);

/// 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 {
jyameo marked this conversation as resolved.
Show resolved Hide resolved
await _initialize();
return _soundNullSafety;
return true;
}

/// A list of all libraries in the Dart application.
Expand Down Expand Up @@ -178,8 +177,6 @@ class MetadataProvider {

Future<void> _initialize() async {
await _metadataMemoizer.runOnce(() async {
var hasSoundNullSafety = true;
var hasUnsoundNullSafety = true;
// The merged metadata resides next to the entrypoint.
// Assume that <name>.bootstrap.js has <name>.ddc_merged_metadata
if (entrypoint.endsWith('.bootstrap.js')) {
Expand All @@ -199,22 +196,14 @@ class MetadataProvider {
final metadata =
ModuleMetadata.fromJson(moduleJson as Map<String, dynamic>);
_addMetadata(metadata);
hasUnsoundNullSafety &= !metadata.soundNullSafety;
hasSoundNullSafety &= metadata.soundNullSafety;
_logger
.fine('Loaded debug metadata for module: ${metadata.name}');
} catch (e) {
_logger.warning('Failed to read metadata: $e');
rethrow;
}
}
if (!hasSoundNullSafety && !hasUnsoundNullSafety) {
throw Exception('Metadata contains modules with mixed null safety');
}
_soundNullSafety = hasSoundNullSafety;
}
_logger.info('Loaded debug metadata '
'(${_soundNullSafety ? "sound" : "weak"} null safety)');
}
});
}
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
60 changes: 26 additions & 34 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,7 +118,12 @@ class SdkConfiguration {
path == null ? null : p.toUri(p.absolute(path));

Uri? get sdkDirectoryUri => _toUri(sdkDirectory);
Uri? get sdkSummaryUri => _toUri(sdkSummaryPath);

@Deprecated('Only sound null safety is supported as of Dart 3.0')
Uri? get soundSdkSummaryUri => _toUri(soundSdkSummaryPath);
jyameo marked this conversation as resolved.
Show resolved Hide resolved

@Deprecated('Only sound null safety is supported as of Dart 3.0')
Uri? get weakSdkSummaryUri => _toUri(weakSdkSummaryPath);

/// Note: has to be ///file: Uri to run in an isolate.
Expand All @@ -139,28 +149,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);
});
}
25 changes: 0 additions & 25 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, false);
});

test('throws on metadata with absolute import uris', () async {
Expand Down Expand Up @@ -114,6 +90,5 @@ void main() {
expect(parts.length, 1);
expect(parts[0], 'org-dartlang-app:///web/main.dart');
}
expect(metadata.soundNullSafety, false);
});
}
Loading