Check for Unused Properties on Annotated Schema Components #1366
Conversation
- currently we check unused properties on terms only, which means unsused properties on non-term schema components can be missed. This will fix that issue, by moving the checks to AnnotatedSchemaComponents instead. - fix bug with XercesSchemaFileLocation.equals assuming all incoming objects would be XercesSchemaFileLocations as well. We also get just plain SchemaFileLocations, which can break things, so instead we compare against SchemaFileLocation members - add tests DAFFODIL-2798
| } | ||
| } | ||
|
|
||
| val descendantsForCheck = this match { |
There was a problem hiding this comment.
There are lists of all schema components elsewhere. You do not need to do your own recursive traversal to obtain them.
SchemaSet.allSchemaComponents is a Seq[SchemaComponent]
There was a problem hiding this comment.
So I actually looked into that, but becauseit is a Seq[SchemaComponent], and not an AnnotatedSchemaComponent, it doesn't know what checkUsedProperties is. Attempts to move checkUsedProperties to SchemaComponent instead resulted in issues since things like formatAnnotations that we use to grab the properties only exist in AnnotatedSchemaComponent.
There was a problem hiding this comment.
Can't you just call the method checkUsedProperties on the schema components that happen to be AnnotatedSchemaComponents? E.g.,
schemaSet.allSchemComponents.collect{ case asc: AnnotatedSchemaComponent => asc.checkUsedProperties }
There was a problem hiding this comment.
Done. Also added propagating the elements down from referencer to referencee
There was a problem hiding this comment.
And from simple type to element (if it has any used properties) and from element to simple type (but only for the properties the simple type carries)
- fix tests with unncessery properties - update TransitiveClosureSchemaComponents to include EnumerationDef - use root.allComponents to call checkUnusedProperties - use root.allComponents to propagate used properties of a referencer element to the referenced element DAFFODIL-2798
- update CheckUsedProperties comment DAFFODIL-2798
…Components - copy used properties from type to element and element to type - only copy properties if the element or it's simpletype have the property - update test DAFFODIL-2798
| val std = ost.get.asInstanceOf[SimpleTypeDefBase] | ||
| if (std.propCache.nonEmpty) { | ||
| // if it has used properties transfer it to the element that's using it | ||
| // we don't need the check for what is carries since all properties on the type are shared |
There was a problem hiding this comment.
typo in comment "is carries" -> "it carries"
| case _ => // do nothing | ||
| } | ||
| } | ||
|
|
||
| private lazy val checkUnusedProperties = LV('hasUnusedProperties) { |
There was a problem hiding this comment.
Needs same scaladoc warning as is on the AnnotatedSchemaComponent val of the same name. This pass must happen after all other compilation has been done.
| private lazy val propagateReferenceeAndTypeUsedProperties = { | ||
| root.allComponents.collect { | ||
| case ref: AbstractElementRef => | ||
| val referent = ref.optReferredToComponent.get |
There was a problem hiding this comment.
Is a referent the one doing the referring, or the one referred to? Ambiguous to me. Explain val referent with a comment.
| @@ -540,8 +540,51 @@ final class SchemaSet private ( | |||
| .flatMap(_.defineVariables) | |||
| .union(predefinedVars) | |||
|
|
|||
| // propagated to referenced element propCache | |||
There was a problem hiding this comment.
Please give real scaladoc to this method and explain the problem (which I think is that a simpleType could have properties that we only know are used due to an element decl that references that type.
Explain the algorithm being used to determine properties that are unused. The way you are using and populating the propCache to get this answer and how information moves from one object's propCache to another is needs explanation.
The code here is simpler than I expected. Nothing is chasing the simpleType base chains for example, and there is some reason why we don't have to do that. You have a case for element refs and one for element decls, but nothing for simple types?
| // after compilation is done, we want to walk through all our refs and | ||
| // if a property is in the cache of the referencer, put it into the | ||
| // cache of the referenced element | ||
| propagateReferenceeAndTypeUsedProperties |
There was a problem hiding this comment.
I wonder if it would be possible or benefiit do this propogation at the time of property lookup instead of a step at the end of compilation? For example, is it possible for findPropertyOption to add to more than just one cache? It could add to its current cache but also use this logic to find any element reference/simpletypes and add to their cache as well? That way all the cache stuff is in one spot?
Or maybe alternatively, could checkUnusedProperties be updated to inspect the referenced propCaches instead of just its, and then we don't have to propagate/copy things at all?
There was a problem hiding this comment.
So I considered doing at the time of property lookup, but we do that in daffodil-lib which doesn't have access to the daffodil-lib classes like ElementRef that'd make it possible to find the element refs/simple types.
For the 2nd suggestion, we run into the same issues where 2 element refs share the element they are referencing but only the first element ref 's propCache show it as using the properties, not the referenced element and not the 2nd element ref. Which is why we opted for propagation
There was a problem hiding this comment.
daffodl-core classes like ElementRef*
| @@ -1164,7 +1164,7 @@ | |||
| </xs:complexType> | |||
| </xs:element> | |||
| <xs:group name="g0"> | |||
| <xs:sequence dfdl:separatorPosition="infix"> | |||
| <xs:sequence> | |||
There was a problem hiding this comment.
Can group sequences really not have any properties? I thought that was legal?
There was a problem hiding this comment.
They can have properties and those are combined with those on the group reference to form the combined set of properties for that sequence.
There was a problem hiding this comment.
Then why did dfdl:separatorPosition need to be removed from these group sequences? I assumed it was removed because it was creating a unused property warning, but it seems like the property should be used?
There was a problem hiding this comment.
It was indeed creating an unused property warning, but since the default separatorPosition is infix. I deemed them unnecessary and safe to remove.
There was a problem hiding this comment.
Seems like that shouldn't create a warning. Even though it's the same as the default the property should still be used. It just means the default format won't be used. Maybe there's an additional code change needed?
There was a problem hiding this comment.
Ok, I'll add them back in and investigate what's going on
DAFFODIL-2798