feat(core): complete the internal representation (imports, model file, model manager)#12
feat(core): complete the internal representation (imports, model file, model manager)#12yash-kumarx wants to merge 5 commits into
Conversation
Type a model's imports (wildcard, named, aliased), and group one namespace's declarations and imports into a ModelFile that resolves the names it can see locally. Signed-off-by: Yash Kumar <yashr17042006@gmail.com>
Resolve types across namespaces, walk the inheritance chain to collect a type's properties, and check assignability. Preload the concerto@1.0.0 system model that every user type extends. Signed-off-by: Yash Kumar <yashr17042006@gmail.com>
|
Ah the issue references are not working by just number references, because they are on the main repo :( you can add direct links. |
ekarademir
left a comment
There was a problem hiding this comment.
I've reviewed about half way to model file. Left some comments that would simplify your life. You can address them until I finish the rest.
| Type { | ||
| /// The namespace the type is imported from. | ||
| namespace: String, | ||
| /// The imported type's short name. |
There was a problem hiding this comment.
It's not the shortname it's the type.
There was a problem hiding this comment.
Fixed that, doc now reads "The name of the imported type."
| Types { | ||
| /// The namespace the types are imported from. | ||
| namespace: String, | ||
| /// The imported short type names. |
There was a problem hiding this comment.
The same here, they are types
|
|
||
| /// `true` for a wildcard (`ns.*`) import, which can only be resolved by | ||
| /// consulting the declarations of the imported namespace. | ||
| pub fn is_wildcard(&self) -> bool { |
There was a problem hiding this comment.
With the new version of concerto, wildcard imports are throwing an error https://github.com/accordproject/concerto/blob/542b630a18b3febaffd67f94a366d4b6175fa79b/packages/concerto-core/src/introspect/modelfile.ts#L745
Maybe we can also do that instead of trying to make them work.
| match self { | ||
| Self::All { .. } => None, | ||
| Self::Type { namespace, name } if name == short => Some(qualify(namespace, name)), | ||
| Self::Type { .. } => None, |
There was a problem hiding this comment.
I think this corresponds to non-strict mode. Which is another thing we don't allow with v4 https://github.com/accordproject/concerto/blob/542b630a18b3febaffd67f94a366d4b6175fa79b/packages/concerto-core/src/introspect/modelfile.ts#L700
There was a problem hiding this comment.
Looks like there was some code that got in for non-strict code with the previous PR that allows non-strict mode
You can remove them as well. Or in a separate PR.
There was a problem hiding this comment.
The non-strict path in our code was the wildcard resolution, so removing that makes resolution strict: only primitives, local declarations, and named imports resolve now
There was a problem hiding this comment.
Looks like there was some code that got in for non-strict code with the previous PR that allows non-strict mode
You can remove them as well. Or in a separate PR.
Versions are mandatory in v4, so I'll strip the version-insensitive matching in a separate PR.
Concerto v4 disallows wildcard imports and non-strict resolution. Reject `import ns.*` while parsing (this was the non-strict path) and drop the wildcard handling in Import and the model manager. Also fix the import field docs to say 'type' rather than 'short name'. Signed-off-by: Yash Kumar <yashr17042006@gmail.com>
|
I've addressed all of these in 4e1e484, thanks |
| } | ||
|
|
||
| /// True if this is the built-in `concerto` system namespace. | ||
| pub fn is_system(&self) -> bool { |
There was a problem hiding this comment.
this is a bit ambiguous, it could be more clear
| pub fn is_system(&self) -> bool { | |
| pub fn is_system_namespace(&self) -> bool { |
| /// Resolves a short name from what this file declares or imports: the | ||
| /// primitives, its own declarations, and its named imports. Returns `None` | ||
| /// if the name is none of those. | ||
| pub fn resolve_local(&self, short: &str) -> Option<String> { |
There was a problem hiding this comment.
This could also be more clear
| pub fn resolve_local(&self, short: &str) -> Option<String> { | |
| pub fn resolve_local_type(&self, short: &str) -> Option<String> { |
| } | ||
|
|
||
| /// The `concerto@1.0.0` system model, as a JSON AST. | ||
| pub fn root_model_ast() -> serde_json::Value { |
There was a problem hiding this comment.
This is not correct. All it's doing is serialising some JSON types. Instead it should directly use the JSON file here https://github.com/accordproject/concerto-metamodel/blob/main/lib/metamodel.json (or better, the raw version https://raw.githubusercontent.com/accordproject/concerto-metamodel/refs/heads/main/lib/metamodel.json)
This could be part of the concerto-metamodel crate, instead of core.
There was a problem hiding this comment.
Vendored the canonical JSON instead of building the AST in code.
One note: metamodel.json defines concerto.metamodel@1.0.0, the metamodel types themselves. The file that defines concerto@1.0.0 is rootmodel.json in concerto-core, so that's the one vendored here, unchanged.
There was a problem hiding this comment.
Moving it into the concerto-metamodel crate would conflict with the build script PR right now, but I can do that as a follow-up once both merge. Preloading the metamodel namespace itself I'd defer to the validation work, where it's actually needed.
Signed-off-by: Yash Kumar <yashr17042006@gmail.com>
Loads concerto@1.0.0 from the rootmodel.json that ships with concerto-core instead of building the AST in code. Signed-off-by: Yash Kumar <yashr17042006@gmail.com>
Closes #1264, closes #1269
Finishes the internal representation, on top of #9.
Changes
Import: typed model imports (wildcardns.*, named, and aliased).ModelFile: owns one namespace's declarations and imports, indexes them, andresolves names it can see locally (primitives, local types, named imports).
ModelManager: loads model files and resolves types across namespaces(incl. wildcard imports), walks the inheritance chain to collect a type's
properties, and checks assignability. Preloads the
concerto@1.0.0system model.$classand non-arrayimports/declarations).Flags
from_jsontrait (plus a derive macro inconcerto-macros) is asensible follow-up, as discussed; kept as methods here.
Related Issues
Author Checklist
--signoffoption of git commit.mainfromfork:yash/introspection-rest