Union fixes #137
Union fixes #137
Conversation
Core/Meta/Definition.cs
Outdated
| /// <summary> | ||
| /// List of definitions enclosing this definition, from outer to inner. Empty if this is a top level definition. | ||
| /// </summary> | ||
| public List<Definition>? Scope | ||
| { | ||
| get | ||
| { | ||
| if (Parent is null) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Remove the ? from the return type, and remove the if (Parent is null) { return null; }, and just let this method return an empty list for a top-level definition like the doc-comment says (the while-loop body will run 0 times 👍).
There was a problem hiding this comment.
Ah, oversight.
Core/Parser/SchemaParser.cs
Outdated
| var scope = _scopes.Last(); | ||
| _scopes.Remove(scope); |
There was a problem hiding this comment.
This makes me think _scopes should be a Stack<T>. Maybe that also nicely captures how it is actually used (i.e. even conceptually, it's a stack of scopes).
There was a problem hiding this comment.
You're right, I'm using that list just like a stack lol. I've got a bad case of javascript-brain where you have to use normal arrays for every kind of linear data structure.
Core/Parser/SchemaParser.cs
Outdated
| // It might be better for this to go inside BebopSchema.Validate but it's much simpler if I can use the typeReferences map | ||
| if (referenceScope is not null && referenceScope.Find((parent) => parent is UnionDefinition) is UnionDefinition union) | ||
| { | ||
| if (definitionScope is null || !definitionScope.Contains(union)) | ||
| { |
There was a problem hiding this comment.
You can get rid of these null checks after applying the change in my earlier comment about Scope.
| /* | ||
| foreach (var definition in _definitions.Values) | ||
| { | ||
| var scope = definition.Scope; | ||
| var dependencies = definition.Dependencies(); | ||
|
|
||
|
|
||
| foreach (var dependency in dependencies) | ||
| { | ||
| var depScope = _definitions[dependency].Scope; | ||
| if (depScope is not null && depScope.Find((parent) => parent is UnionDefinition) is UnionDefinition union) { | ||
| if (scope is null || !scope.Contains(union)) | ||
| { | ||
| throw new ReferenceScopeException(definition, _definitions[dependency], "union"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| }*/ |
There was a problem hiding this comment.
My classic review comment about commented-out code:
If there's a good reason to keep it around but commented-out, add a comment explaining why.
If there isn't a good reason, let's get rid of it.
Make nested unions and references to types defined in union branches invalid. Also add mstest tests for the bebop compiler core. These tests ensure the valid schemas in the Laboratory/Schemas directory do compile, and the new invalid ones I added in ShouldFail do not. I had to modify the union test schema as evidently it included nested unions which we're no longer supporting.
The checks are done in SchemaParser, which is debatably not the right place for checking references, but having access to the _typeReferences set that's only available there makes the logic a lot cleaner. It will need to be moved once we add support for multiple errors per compiler run, however. Maybe add that set to BebopSchema?