hexabits commented 6 years ago

While rewriting nifxml.py I have written some linting functions to catch issues that simple stuff like XSD and DTD cannot.

Error Levels

CRT = Critical ERR = Error WRN = Warning INF = Info

For this issue I will refer to: ver1 as minver ver2 as maxver

General Issues

CRT Missing name for XML tags: basic, compound, niobject, enum, bitflags, token
CRT Missing id for XML tags: version

Member-type Issues

Collision issues

ERR Member names that are also type names (compound, NiObject, enum, etc).
- Member names cannot be class/enum names in case of clashing in the generated language, e.g. PascalCase is used for both members and classes.
ERR Expression attributes that are using type names.
- (With cond="T" and cond="!T" being replaced in #76 this can be done)
ERR Cannot have both onlyT and excludeT (See #76)
WRN Should not have cond alongside onlyT/excludeT.
- Reason for this is that the functionality of only/exclude used to be wrapped up in cond, and technically an easy way of adapting existing code for the new attributes is to alias it back to cond.
WRN Members that have (minver or maxver etc.) and vercond
- If vercond is present, no other version-specific attributes should exist as it spreads out the version conditions and reduces readability and maintainability.
- There are use cases where minver/maxver/vercond together would be beneficial, such as
```
<add name="Switch State" type="bool" minver="10.1.0.106" vercond="User Version 2 &lt; 130" />
```
  As it's clear enough that the starting version is 10.1.0.106 and also User Version 2 has to be < 130.
- Cases where this should NOT be allowed is you have Version comparisons in the vercond which means you're splitting checks on the Version across 3 attributes, e.g. ver1="X" ver2="Y" vercond="Version != Z"

Not Enough Information Issues

CRT Missing name or type
- Members need both name and type.
ERR type that is_template but empty template string
- A template type needs its template parameter.

Incorrect Information Issues

ERR name or type do not match identifier regex
- This attribute has an invalid identifier, e.g. leading numbers, trailing spaces, invalid characters.
ERR template but no type or unexpected type
- The Member does not provide a correct template type but has template filled out.
ERR type that should not have a default set
- Some objects have no reason to provide a default value
ERR type that is Ref/Ptr but template is not a NiObject
- A Ref/Ptr needs a template type that is an NiObject
ERR Type in onlyT/excludeT must be an NiObject and must inherit the member parent NiObject.
- These attributes are meant to exclude members from certain subclasses of the NiObject.
ERR default contains a comma-separated list that is not parseable into a tuple of floats or ints.
WRN default could not be parsed into an enumerated value, int, float, or tuple of int/floats.
- There are currently no defaults that evaluate to strings or non-numeric values. However, default strings could eventually be implemented for certain types so this will just be a warning until that time.
- Edit: Was incorrect about the above. Default strings are used in BSConnectPoint.

Redundant Information Issues

WRN Members named "Unknown" that also have a description
WRN Members that do not appear in any <version>

Reference Issues

ERR Expression for vercond has identifier that is not a global name
- vercond expression references something other than the global version identifiers from the Header.
WRN Referenced name does not always exist in some capacity for all of this Member's versions
- Anything used as a reference in expressions needs to be resolveable for all rows that this Member belongs to. So you must verify they always coexist.
WRN Expression for cond, arr1, arr2, arg has identifier that is not a local name (Compound)
WRN Expression for cond, arr1, arr2, arg has identifier that is not a local name (NiObject)
WRN Expression for cond, arr1, arr2, arg has identifier that is not a local or inherited name (NiObject)
For expressions where the string is solely a reference (no logic or arithmetic):
- WRN Referenced member for cond is not a boolean type
- WRN Referenced member for arr1 is not an integral type
- WRN Referenced member for arr2 is not an integral type or an integral array type
  - NOTE: Passing in arrays to arr2 is currently up for review, but as it currently stands, doing so is both valid and necessary (Strip Lengths for tri strips for example)

All of these must also filter out various tokens like #ARG# or #T# before doing checks.

Duplicate Issues

ERR Duplicate name that appear in the same version. For NiObject, check inheritance.
ERR Duplicate name that differ in castable type but do not have a suffix, regardless of version exclusivity
- Any duplicate names without a suffix have to have castable type. For
```
<add name="Value 1" type="uint" version="..." />
<add name="Value 1" type="ushort" version="..."  />
```
  Value 1 casts from ushort->uint here, because the member that gets stored is of the first type, uint. A ushort can fit inside a uint, so there is no problem with this naming collision so long as they are version exclusive. But a short cannot come before an int as the data member will be declared as a short, and any int value will be truncated when trying to store in that field.

Enum-type Issues

ERR Option name or prefix that do not start with an alpha character
- Even if using a Prefix, the enum name should be a valid identifier.
ERR default that do not exist in the enum options
- Compares the default listed in a Member to the actual enum options to verify it exists.
WRN Inconsistent use of prefix/no-prefix enum strings. Prefer prefix-less.
- The default listed in a Member should be the prefix-less enum string.
WRN default that use integers instead of enumeration strings (excluding bitflags)
- Disallow use of integers as defaults for enums (but not bitflags, for now)

I've mostly written these from violations actually found in nif.xml while writing the linter, so this list is going to keep expanding as I add more linting. I will post examples of all the errors I've caught whie developing it.

Ghostwalker71 commented 6 years ago

Cases where this should NOT be allowed is you have Version comparisons in the vercond which means you're splitting checks on the Version across 3 attributes.

Could you give an example for this, I'm not sure I fully grasp the intent.

hexabits commented 6 years ago

Exactly what I said: Splitting Version checks across 3 attributes. I've managed to remove most of the offending examples over time, but a few still remain that have 2 checks across attributes:

ver1="10.1.0.0" vercond="!((Version == 20.2.0.7) &amp;&amp; (User Version 2 &gt; 0))"

Firstly, ((Version == 20.2.0.7) && (User Version 2 > 0)) is a construct that is repeated several times throughout the XML now, which means "Bethesda 20.2", and as demonstrated in #70 will become instantaneously more readable and understandable once it's turned into a token.

But ignoring the tokens for now, add in more version attributes and it becomes a confusing mess:

ver1="10.1.0.0" ver2="20.5.0.0" vercond="!((Version == 20.2.0.7) &amp;&amp; (User Version 2 &gt; 0))"
ver1="10.1.0.0" ver2="20.5.0.0" userver="0"  vercond="!((Version == 20.2.0.7) &amp;&amp; (User Version 2 &gt; 0))"

So you now have 3+ attributes to deal with, all which have some check on Version etc.. Maybe those checks do not conflict but maybe they do. It should just be written as an expression.

Once tokenized however, it's not too bad, but it should still be avoided:

ver1="10.1.0.0" ver2="20.5.0.0" vercond="!#BS202#"

So, as of right now, there are 44 members that the linter warns on for having vercond in addition to other ver attributes:

WARNING:nifxml:ControlledBlock: 'Priority' has vercond in addition to other ver attributes.
WARNING:nifxml:NiBound: 'Unknown 13 Shorts' has vercond in addition to other ver attributes.
WARNING:nifxml:Morph: 'Legacy Weight' has vercond in addition to other ver attributes.
WARNING:nifxml:HavokFilter: 'Layer' has vercond in addition to other ver attributes.
WARNING:nifxml:HavokMaterial: 'Material' has vercond in addition to other ver attributes.
WARNING:nifxml:RagdollDescriptor: 'Motor' has vercond in addition to other ver attributes.
WARNING:nifxml:LimitedHingeDescriptor: 'Motor' has vercond in addition to other ver attributes.
WARNING:nifxml:PrismaticDescriptor: 'Motor' has vercond in addition to other ver attributes.
WARNING:nifxml:bhkRigidBody: 'Unknown Int 2' has vercond in addition to other ver attributes.
WARNING:nifxml:bhkRigidBody: 'Penetration Depth' has vercond in addition to other ver attributes.
WARNING:nifxml:NiAVObject: 'Flags' has vercond in addition to other ver attributes.
WARNING:nifxml:NiDynamicEffect: 'Switch State' has vercond in addition to other ver attributes.
WARNING:nifxml:NiDynamicEffect: 'Num Affected Nodes' has vercond in addition to other ver attributes.
WARNING:nifxml:NiDynamicEffect: 'Affected Nodes' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeomMorpherController: 'Num Unknown Ints' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeomMorpherController: 'Unknown Ints' has vercond in addition to other ver attributes.
WARNING:nifxml:NiBoneLODController: 'Num Shape Groups' has vercond in addition to other ver attributes.
WARNING:nifxml:NiBoneLODController: 'Shape Groups 1' has vercond in addition to other ver attributes.
WARNING:nifxml:NiBoneLODController: 'Num Shape Groups 2' has vercond in addition to other ver attributes.
WARNING:nifxml:NiBoneLODController: 'Shape Groups 2' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeometry: 'Skin Instance' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeometry: 'Material Data' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeometry: 'Material Data' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeometryData: 'Vector Flags' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeometryData: 'Has Unk Floats' has vercond in addition to other ver attributes.
WARNING:nifxml:NiGeometryData: 'Unk Floats' has vercond in addition to other ver attributes.
WARNING:nifxml:NiParticlesData: 'Radii' has vercond in addition to other ver attributes.
WARNING:nifxml:NiParticlesData: 'Rotations' has vercond in addition to other ver attributes.
WARNING:nifxml:NiParticlesData: 'Rotation Axes' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSysData: 'Rotation Speeds' has vercond in addition to other ver attributes.
WARNING:nifxml:NiSequence: 'Num Unknown Ints' has vercond in addition to other ver attributes.
WARNING:nifxml:NiSequence: 'Unknown Ints' has vercond in addition to other ver attributes.
WARNING:nifxml:NiSequence: 'Unknown Ref' has vercond in addition to other ver attributes.
WARNING:nifxml:NiControllerSequence: 'Anim Notes' has vercond in addition to other ver attributes.
WARNING:nifxml:NiControllerSequence: 'Num Anim Note Arrays' has vercond in addition to other ver attributes.
WARNING:nifxml:NiControllerSequence: 'Anim Note Arrays' has vercond in addition to other ver attributes.
WARNING:nifxml:NiMesh: 'Has Extra EM Data' has vercond in addition to other ver attributes.
WARNING:nifxml:NiMesh: 'Extra EM Data' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSParticleSystem: 'Unk Int' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSParticleSystem: 'Unk Byte' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSSimulatorGeneralStep: 'Unk Byte' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSGravityFieldForce: 'Unk Short' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSGravityFieldForce: 'Unk Byte' has vercond in addition to other ver attributes.
WARNING:nifxml:NiPSVolumeEmitter: 'Unk Byte' has vercond in addition to other ver attributes.

Which is 44 instances, which is around 1/4th of all of the vercond usages. Thankfully, since rewriting basically every vercond, most of them only contain a single check against User Version 2 (which, also, is being renamed since it's not even correct).

I may demote the warning to info, if the vercond only contains a single check against User Version / BS Version, as that is still mostly readable. But that also assumes we will for sure be getting rid of userver and userver2 attributes because the same collisions apply there.. Having User Version referenced in both userver and vercond is confusing.

Ghostwalker71 commented 6 years ago

Thank you for the clarification.

Candoran2 commented 2 years ago

Concerning default strings as used as used in BSConnectPoint: How can a string be distinguished from any other type? Is it on the field type to handle that? If they can't be distinguished, would it make sense to use "/" as a string denoter?

niftools / nifxml

[Spec] nifxml linter #74