Stream: fhirpath
Topic: Reserved words
Grahame Grieve (Oct 02 2019 at 22:38):
GF#23073 - I'm interested in comment on this. I agree with the comment in the resolution, but I'm not convinced that it's conformant
Lee Surprenant (Oct 02 2019 at 22:43):
I discussed this one with @Bryn Rhodes and Ewout for a bit after connectathon. @Ewout Kramer said they had to work around this violation of the fhirpath grammar in their implementation as well. I think he's out now, but perhaps someone else can fill in for him on that side?
Lee Surprenant (Oct 02 2019 at 22:53):
I've also had pretty extensive discussion with @John Timm on this one and here were my notes:
- http://hl7.org/fhirpath/2018Sep/index.html#keywords says "In general, keywords within FHIRPath are also considered reserved words, meaning that it is illegal to use them as identifiers. If necessary, identifiers that clash with a reserved word can be quoted."
Grahame Grieve (Oct 02 2019 at 22:54):
that's not the same as 'must'
Lee Surprenant (Oct 02 2019 at 23:03):
was gonna add more points from my notes, but they are sort of contingent on keywords being reserved / illegal as identifiers :-)
Lee Surprenant (Oct 02 2019 at 23:03):
agree that language is unusually loose
Lee Surprenant (Oct 02 2019 at 23:06):
In general, keywords within FHIRPath are also considered reserved words, meaning that it is illegal to use them as identifiers.
does it mean
"keywords are generally considered to be reserved words and therefor it is always illegal to use them as identifiers"
OR
"keywords are usually considered to be reserved words and therefor should be generally avoided as identifiers (but not required to be quoted)"
Grahame Grieve (Oct 02 2019 at 23:11):
yes
Bryn Rhodes (Oct 03 2019 at 02:00):
Yes, that language could definitely be clarified. The intent of that wording was to allow for the fact that the grammar defines the in, is, as, and contains keywords as possible identifiers, specifically because they are also function names and so need to be available as identifiers.
John Timm (Oct 03 2019 at 14:01):
+1 to clarify the language. My biggest concern is that there are structure definitions that include constraints with FHIRPath expressions that do not escape some of the problematic keywords (i.e. div and contains). Even if it is a SHOULD and not a SHALL, I think that the base specification should observe some sort of best practice when specifying constraints in a StructureDefinition. Otherwise its left up to the implementer to address. -- especially if the published ANLR grammar is used as a basis for an expression parser. The grammar will not work out-of-the-box unless identifiers that conflict with keywords are delimited.
Bryn Rhodes (Oct 03 2019 at 15:50):
The grammar explicitly defines contains, in, is, and as as keywords, so they do not need to be escaped in paths. div, mod, and, or, xor, and implies would need to be escaped, but there are no function name clashes with any of those keywords. Are there any element name clashses?
Bryn Rhodes (Oct 03 2019 at 15:51):
And I just validated to be sure, the constraint expression referenced in the above tracker _does_ parse correctly: ValueSet.expansion.contains.code
Paul Lynch (Oct 03 2019 at 15:54):
ValueSet.expansion.contains.code
That also works in fhirpath.js, which generates its parser from the grammar.
Bryn Rhodes (Oct 03 2019 at 16:13):
GF#24895 to clarify the language
John Timm (Oct 03 2019 at 19:11):
@Bryn Rhodes Let me clarify my previous comments by providing more detail. I am using an ANLR4 generated Java parser and the expression ValueSet.expansion.contains.code causes issues after it is parsed into an abstract syntax tree. If you process the AST using the generated parse tree visitor, it will incorrectly call visitMembershipExpression(FHIRPathParser.MembershipExpressionContext ctx) This tells me that it thinks that an unescaped contains identifier refers to the contains operator. It's pretty easy to replicate by downloading antlr and running it on fhirpath.g4. I could put together a unit test if you'd like.
Paul Lynch (Oct 03 2019 at 19:14):
ValueSet.expansion.contains.codeThat also works in fhirpath.js, which generates its parser from the grammar.
I just realized that fhirpath.js is still using the STU1 grammar file. Perhaps differences between that and the new one account for why the above expression parses correctly for fhirpath.js but not for @John Timm.
Bryn Rhodes (Oct 03 2019 at 19:30):
@John Timm , what version of the FHIRPath grammar file are you reproducing that with?
Bryn Rhodes (Oct 03 2019 at 19:32):
If you're using this one: http://hl7.org/fhirpath/grammar.html, agreed, but that has been updated in the latest balloted version: http://hl7.org/fhirpath/2019May/grammar.html
Bryn Rhodes (Oct 03 2019 at 19:32):
Which is currently on track for normative publication.
John Timm (Oct 03 2019 at 19:33):
@Bryn Rhodes The version I'm using is here:
http://hl7.org/fhirpath/2018Sep/grammar.html
Bryn Rhodes (Oct 03 2019 at 19:33):
Yeah, that one still has the issue, it was found and addressed as part of the most recent ballot.
Bryn Rhodes (Oct 03 2019 at 19:34):
Apologies, I should have guessed that was what was going on. :(
John Timm (Oct 03 2019 at 19:34):
Okay, what has changed in the latest version that addresses the issue?
John Timm (Oct 03 2019 at 19:34):
I will be sure to upgrade ASAP.
Bryn Rhodes (Oct 03 2019 at 19:34):
The identifier production rule:
identifier
: IDENTIFIER
| DELIMITEDIDENTIFIER
| 'as'
| 'contains'
| 'in'
| 'is'
;
Bryn Rhodes (Oct 03 2019 at 19:35):
It specifically calls out contains and in, where in previous versions, we had only called out is and as.
John Timm (Oct 03 2019 at 19:35):
Nice
John Timm (Oct 03 2019 at 19:35):
I think div has the same problem
John Timm (Oct 03 2019 at 19:36):
as div is an operator and used in a Narrative constraint
Bryn Rhodes (Oct 03 2019 at 19:39):
Ah, because Narrative has a div element.
Bryn Rhodes (Oct 03 2019 at 19:39):
I wasn't aware of that clash. In that invariant, yes, div would need to be escaped.
Bryn Rhodes (Oct 03 2019 at 19:43):
Added this as followup to that tracker.
John Timm (Oct 03 2019 at 19:53):
@Bryn Rhodes Thank you!
Grahame Grieve (Oct 03 2019 at 21:24):
which constraint is this? I don't see it?
Bryn Rhodes (Oct 03 2019 at 21:50):
DomainResource: dom-6
Grahame Grieve (Oct 03 2019 at 22:03):
if someone will make a task, I'll fix that one in the R4 technical correction
John Timm (Oct 04 2019 at 00:43):
And also txt-1 and txt-2 on Narrative
Grahame Grieve (Oct 04 2019 at 01:21):
I'm not seeing that
Bryn Rhodes (Oct 04 2019 at 02:47):
I updated the proposed resolution to GF#23073 to update constraint dom-6 on DomainResource. Does that work or do you need a separate task?
Grahame Grieve (Oct 04 2019 at 02:49):
no that'll be fine
John Timm (Oct 04 2019 at 13:40):
@Grahame Grieve Taken directly from the Narrative StructureDefinition:
{
"id": "Narrative.div",
"path": "Narrative.div",
"short": "Limited xhtml content",
"definition": "The actual narrative content, a stripped down version of XHTML.",
"comment": "The contents of the html element are an XHTML fragment containing only the basic html formatting elements described in chapters 7-11 and 15 of the HTML 4.0 standard, <a> elements (either name or href), images and internally contained stylesheets. The XHTML content SHALL NOT contain a head, a body, external stylesheet references, scripts, forms, base/link/xlink, frames, iframes and objects.",
"min": 1,
"max": "1",
"base": {
"path": "Narrative.div",
"min": 1,
"max": "1"
},
"type": [
{
"code": "xhtml"
}
],
"constraint": [
{
"key": "txt-1",
"severity": "error",
"human": "The narrative SHALL contain only the basic html formatting elements and attributes described in chapters 7-11 (except section 4 of chapter 9) and 15 of the HTML 4.0 standard, <a> elements (either name or href), images and internally contained style attributes",
"expression": "htmlChecks()",
"xpath": "not(descendant-or-self::*[not(local-name(.)=('a', 'abbr', 'acronym', 'b', 'big', 'blockquote', 'br', 'caption', 'cite', 'code', 'col', 'colgroup', 'dd', 'dfn', 'div', 'dl', 'dt', 'em', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'hr', 'i', 'img', 'li', 'ol', 'p', 'pre', 'q', 'samp', 'small', 'span', 'strong', 'sub', 'sup', 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'tr', 'tt', 'ul', 'var'))]) and not(descendant-or-self::*/@*[not(name(.)=('abbr', 'accesskey', 'align', 'alt', 'axis', 'bgcolor', 'border', 'cellhalign', 'cellpadding', 'cellspacing', 'cellvalign', 'char', 'charoff', 'charset', 'cite', 'class', 'colspan', 'compact', 'coords', 'dir', 'frame', 'headers', 'height', 'href', 'hreflang', 'hspace', 'id', 'lang', 'longdesc', 'name', 'nowrap', 'rel', 'rev', 'rowspan', 'rules', 'scope', 'shape', 'span', 'src', 'start', 'style', 'summary', 'tabindex', 'title', 'type', 'valign', 'value', 'vspace', 'width'))])"
},
{
"key": "txt-2",
"severity": "error",
"human": "The narrative SHALL have some non-whitespace content",
"expression": "htmlChecks()",
"xpath": "descendant::text()[normalize-space(.)!=''] or descendant::h:img[@src]"
}
],
"isModifier": false,
"isSummary": false,
"mapping": [
{
"identity": "rim",
"map": "N/A"
}
]
}
]
},
Grahame Grieve (Oct 04 2019 at 13:41):
I'm not seeing div in the fhirpath
John Timm (Oct 04 2019 at 13:42):
the path of the constraint is Narrative.div and the constraint is htmlChecks() In order to check this constraint using FHIRPath we either need to first evaluate the path Narrative.div and then htmlChecks() or put the two together into one expression: Narrative.div.htmlChecks()
John Timm (Oct 04 2019 at 13:43):
Either way, with the existing grammar, the generated parser needs an escaped div. I guess there's nothing really that can be changed in the Structure Definition but it is an implementer pitfall
Grahame Grieve (Oct 04 2019 at 13:44):
the path there is the ElementDefinition.path, and it's not a FHIRPath element
John Timm (Oct 04 2019 at 13:45):
Right. Understood. I'm coming at this from the perspective of what actually has to be done in a real implementation. and it is not immediately obvious to an implementer that div actually does need to be escaped in this particular case. So I think the documentation fix will help there.
Lee Surprenant (Oct 08 2019 at 01:50):
Bryn Rhodes The version I'm using is here:
http://hl7.org/fhirpath/2018Sep/grammar.html
Its worth mentioning that this is the version of FHIRPath pointed to by FHIR R4. Is it possible to update http://hl7.org/fhir/R4B/fhirpath.html (and https://build.fhir.org/fhirpath.html) to point at a more recent version of FHIRPath? Is there already a tracker for that? Should we open one?
Grahame Grieve (Oct 08 2019 at 03:19):
it will automatically update to the latest. Neatly, it looks like this will align with the technical correction
Last updated: Apr 12 2022 at 19:14 UTC
