Skip to content

Remove dfdlx:inputTypeCalc and dfdlx:outputTypeCalc#1070

Merged
stevedlawrence merged 1 commit intoapache:mainfrom
stevedlawrence:daffodil-2211-remove-input-output-type-calc
Aug 24, 2023
Merged

Remove dfdlx:inputTypeCalc and dfdlx:outputTypeCalc#1070
stevedlawrence merged 1 commit intoapache:mainfrom
stevedlawrence:daffodil-2211-remove-input-output-type-calc

Conversation

@stevedlawrence
Copy link
Copy Markdown
Member

@stevedlawrence stevedlawrence commented Aug 18, 2023

This removes the dfdlx:inputTypeCalc and dfdlx:outputTypeCalc properties and DPath functions. This allows for removal of a number of related member variables and functions used only by the expression type calc functions. For example, the typeCalcMap is only used by the functions to lookup a calculator by QName at runtime. All type calculators are known statically at compile time, so everything related to runtime type calculator maps goes away.

Also does some minor refactoring:

  • Moves code around so similar logic is all in one place or removes conditionals
  • Changes a tuple where only element has a meaningful value to an Either so we enforce that restriction
  • Fixes typo in IdentityTypeCalc name
  • Changes some error messages so they are a bit more less developer centric

Deprecation/Compatibility:

The dfdlx:inputTypeCalc and dfdlx:outputTypeCalc properties and
functions have been removed. Instead, one should use dfdlx:repType with
dfdlx:repValues and dfdlx:repValueRanges.

DAFFODIL-2211


Note that this is only part of of DAFFODIL-2211. I'm making this a separate PR so it's more clear which parts are about removing input/outputValueCalc and which parts are about updating the diagnostics. It also means if we ever want to bring back typeValueCalc, we can just revert this PR without undoing the diagnostic changes

@stevedlawrence
Copy link
Copy Markdown
Member Author

To do before merged, this needs a Deprecation/Compatibility section in the commit message to go in the release notes.

Copy link
Copy Markdown
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Would benefit from a couple of clarifying comments

Copy link
Copy Markdown
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 This is a good set of changes. Other than the added doc, I just added comments that perhaps there are a few more things that can be removed also, but likely that should be looked at after this is merged.

): Either[Error, DataValuePrimitiveNullable] = Right(x)
}

class UnionTypeCalculator(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this might be able to go away. I'm not understanding any feature of unions. I mean do we allow simple type unions as a repType or can one combine enums with a rep type using unions?

This change set is complex enough, so if you want to put off further investigation of this I'm good with that.

And I also could be entirely mistaken and this stuff may need to be here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we allow simple type unions as a repType or can one combine enums with a rep type using unions?

We allow a simple type to be a union of other simple types which can have repTypes defined. We only have one tdml test for it (and a couple of unit tests) so I'm not sure how heavily it's tested or used, but it seems to work.

Daffodil also supports unions in general (see union.tdml), so maybe it was added for that reason?

If we think this repType unions is unnecessary I can remove it to, I think it would be pretty straightforward and I'd rather do it now while it's all fresh in my memory. But I can do it in a separate PR if that's preferred.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather take these union things out. A repType should just be an integer, and typically isn't going to have facets, just DFDL properties. It's not visible at all in the infoset so there is no notion of an invalid repType value.

If a simple type that uses enumerations and repValue(s) is a union... that feels very problematic to me and I'd like to eliminate that possibility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, are we sure we're not introducing yet another backward incompatibility.. We need to look at the complex schemas that were using inputTypeCalc et al, and see if they're using these union features.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've grepped through our regression suite, and of the handful of schemas that use xs:unions, none of those unions or the simple types they reference use repType. We do use unions, but it looks like they are used to combine various restrictions.

So I don't think we would break anything by removing repType union support. I'll remove union repType support as a separate PR and test the regression suite first to make sure there are no breakages.

This removes the dfdlx:inputTypeCalc and dfdlx:outputTypeCalc properties
and DPath functions. This allows for removal of a number of related
member variables and functions used only by the expression type calc
functions. For example, the typeCalcMap is only used by the functions to
lookup a calculator by QName at runtime. All type calculators are known
statically at compile time, so everything related to runtime type
calculator maps goes away.

Also does some minor refactoring:
- Moves code around so similar logic is all in one place or removes
  conditionals
- Changes a tuple where only element has a meaningful value to an Either
  so we enforce that restriction
- Fixes typo in IdentityTypeCalc name
- Changes some error messages so they are a bit more less developer
  centric

Deprecation/Compatibility:

The dfdlx:inputTypeCalc and dfdlx:outputTypeCalc properties and
functions have been removed. Instead, one should use dfdlx:repType with
dfdlx:repValues and dfdlx:repValueRanges.

DAFFODIL-2211
@stevedlawrence stevedlawrence force-pushed the daffodil-2211-remove-input-output-type-calc branch from 60a1162 to 8ed8eff Compare August 24, 2023 12:19
@stevedlawrence stevedlawrence merged commit 17f3fa8 into apache:main Aug 24, 2023
@stevedlawrence stevedlawrence deleted the daffodil-2211-remove-input-output-type-calc branch August 24, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants