[native_assets_cli] Syntax validation #2115

dcharkes · 2025-03-20T11:49:00Z

This PR adds generated validate functions on the syntax classes that check the type and nullability of the required fields.

This PR does not separate syntax and semantic validation in the ProtocolExtension as proposed in #2088. The inputs/outputs classes expose the underlying JSON and are lazy. So, they don't fail on syntax errors unless traversed.

This PR does not yet address:

[native_assets_cli] input.config.code.android.target_ndk_api should be required in JSON #2039 (follow up PR)
Non-local syntax (e.g. file is required if link_mode is x).

To prevent crashes in the semantic validation, the semantic validation is not run when when syntactic errors are found. To prevent confusion for users, an extra error message is added that semantic validation is not run if syntactic errors are found.

We should consider also skipping extension validation if any errors occur in the base protocol, the validation code of extensions should be able to rely on that base protocol doesn't contain any errors.

github-actions · 2025-03-20T11:50:51Z

PR Health

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbols

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

HosseinYousefi · 2025-03-20T12:37:29Z

pkgs/json_syntax_generator/lib/src/generator/helper_library.dart

+    List<Object?> list,
+    String key,
+  ) {
+    var index = 0;


nit: I'd either use a normal C-style for loop and do final value = list[i] or use for (final (index, value) in list.indexed)

How did I not know about .indexed!

HosseinYousefi · 2025-03-20T12:44:36Z

pkgs/json_syntax_generator/lib/src/generator/helper_library.dart

    throwFormatException(value, T, [key]);
  }

+  List<String> validate<T extends Object?>(String key) {


nit: T extends Object? is the same as just T but it's fine if you want to be more descriptive.

Not as important in generated code but a name like validateIsA better describes the type checking here.

Ah Object? == dynamic I thought it excludes dynamic, but it doesn't.

validate

I wanted to keep it analogous to get. We could do getA, but I feel it makes it less readable.

I think T extends Object? is different to T regarding the avoid_dynamic_calls lint. I might be wrong on this though.

mosuem · 2025-03-20T12:28:59Z

pkgs/json_syntax_generator/lib/src/generator/helper_library.dart

    throwFormatException(value, T, [key]);
  }

+  List<String> validate<T extends Object?>(String key) {


Does it make sense to have a full Dart class in a string? This feels like it should live somewhere the analyzer can look at it...

I wanted to do that initially, but that means that invoking the code generator as a library needs to know where the code generator is so that it can find the file and read it as a String.

If we were to change the code generator to have a bin/ script we would know from the entry point. But we invoke it with a Dart API (and pass in a bunch of objects).

We could change the API to have to pass in URI of the package, but that also seems messy.

I'm open to suggestions.

(Orthogonal to this PR, so we should probably do that in a separate PR.)

Isn't the entry point in hook/tool/generate_syntax.dart, where you could hardcode the path to the file?

I think passing the path to the file is not messy, but your choice.

It makes the Dart API have an extra param and less encapsulated. More ugly in the implementation but less ugly on call sites.

If this package ever gets published on pub, then the user would have to know the path of the package in the pub-cache and pass it in. That feels too messy.

(And publishing a full blown helper package package:json_syntax_runtime_lib like we have with package:jni, seems way too much overkill here.)

Let's keep the ugly inside instead of on the API.

mosuem · 2025-03-20T12:29:51Z

pkgs/native_assets_cli/lib/src/code_assets/syntax.g.dart

@@ -28,6 +28,11 @@ class AndroidCodeConfig {
    json.setOrRemove('target_ndk_api', value);
  }

+  List<String> _validateTargetNdkApi() =>


I would add a header to the file detailing

where it was generated/what generates it

how to regenerate this, and when

Orthogonal to this PR, let me do that in a separate PR

mosuem · 2025-03-20T13:06:39Z

pkgs/native_assets_cli/lib/src/data_assets/validation.dart

@@ -76,6 +91,26 @@ void _validateDataAsset(
  errors.addAll(_validateFile('Data asset ${dataAsset.name} file', file));
 }

+List<String> _validateDataAssetSyntax(EncodedAsset encodedAsset) {


Documentation would be much appreciated ;) List<String> is not a very expressive return type.

Ah, we actually have typedef ValidationErrors. Good catch!

dcharkes · 2025-03-20T13:21:33Z

Thanks @HosseinYousefi & @mosuem !

[native_assets_cli] Syntax validation

fb95354

github-actions bot added the package:native_assets_cli label Mar 20, 2025

dcharkes requested review from HosseinYousefi and mosuem March 20, 2025 11:54

HosseinYousefi approved these changes Mar 20, 2025

View reviewed changes

mosuem reviewed Mar 20, 2025

View reviewed changes

address comments

30913fe

dcharkes merged commit 4e534d3 into main Mar 20, 2025
32 checks passed

dcharkes deleted the syntax-validation branch March 20, 2025 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native_assets_cli] Syntax validation #2115

[native_assets_cli] Syntax validation #2115

dcharkes commented Mar 20, 2025 •

edited

Loading

github-actions bot commented Mar 20, 2025

HosseinYousefi Mar 20, 2025

dcharkes Mar 20, 2025

HosseinYousefi Mar 20, 2025

dcharkes Mar 20, 2025

mosuem Mar 20, 2025

mosuem Mar 20, 2025

dcharkes Mar 20, 2025 •

edited

Loading

mosuem Mar 20, 2025

mosuem Mar 20, 2025

dcharkes Mar 20, 2025

mosuem Mar 20, 2025

dcharkes Mar 20, 2025

mosuem Mar 20, 2025

dcharkes Mar 20, 2025

dcharkes commented Mar 20, 2025

[native_assets_cli] Syntax validation #2115

[native_assets_cli] Syntax validation #2115

Conversation

dcharkes commented Mar 20, 2025 • edited Loading

github-actions bot commented Mar 20, 2025

PR Health

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes commented Mar 20, 2025

dcharkes commented Mar 20, 2025 •

edited

Loading

dcharkes Mar 20, 2025 •

edited

Loading