Skip to content

bug(#221): empty-object allows void attributes#236

Merged
yegor256 merged 9 commits intoobjectionary:masterfrom
h1alexbel:221
Jan 22, 2025
Merged

bug(#221): empty-object allows void attributes#236
yegor256 merged 9 commits intoobjectionary:masterfrom
h1alexbel:221

Conversation

@h1alexbel
Copy link
Member

@h1alexbel h1alexbel commented Jan 13, 2025

In this PR I've adjusted empty-object lint to not warn about void attributes that in fact are empty <o/> in XMIR, and objects that reference that void attributes.
see #221
see #222

@h1alexbel h1alexbel changed the title bug(#221): empty-object allows void attrbiutes bug(#221): empty-object allows void attributes Jan 13, 2025
@h1alexbel
Copy link
Member Author

@maxonfjvipon please check

<xsl:text> is empty. It doesn't have any attributes, neither void nor attached</xsl:text>
</xsl:element>
<xsl:for-each select="/program/objects//o[not(@base='∅') and not(o) and not(text())]">
<xsl:if test="not($void = @base)">
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel I don't really understand what is this comparison for? You've already checked that object does not have @base='∅' in the line above

Copy link
Member Author

@h1alexbel h1alexbel Jan 14, 2025

Choose a reason for hiding this comment

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

@maxonfjvipon here, I check that $void, which is a set of atom names don't contain @base attribute of current object in the iteration.

Consider following XMIR:

<objects>
  <o line="2" name="main" pos="0">
    <o base="" line="2" name="x" pos="1"/>
    <o base="" line="2" name="y" pos="1"/>
    <o base=".print" line="3" name="@" pos="2">
      <o base="stdout" line="4" pos="4">
        <o base="string" line="4" pos="11">48-65-6C-6C-6F-21</o>
        <o base="x" line="4" pos="20"/>
      </o>
    </o>
  </o>
</objects>

In this XMIR, x object referenced to the void attribute x, declared at line 2. This check ensures that atom names ('x', 'y') contain x. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel the original idea of such check was not to have abstract objects that does not have any inner objects, for example:

[] > app
  [] > a1 # empty and useless - should be caught
  if. > @
    true
    [] # kind of useles, but not empty - should not be caught
      2 > @
    1
  [x] > a2 # not empty - should not be caught

So the idea is simple - catch abstract object (which does not have @base attribute) that does not have inner <o> elements

<xsl:text> is empty. It doesn't have any attributes, neither void nor attached</xsl:text>
</xsl:element>
<xsl:for-each select="/program/objects//o[not(@base='∅') and not(o) and not(text())]">
<xsl:if test="not($void = @base)">
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel the original idea of such check was not to have abstract objects that does not have any inner objects, for example:

[] > app
  [] > a1 # empty and useless - should be caught
  if. > @
    true
    [] # kind of useles, but not empty - should not be caught
      2 > @
    1
  [x] > a2 # not empty - should not be caught

So the idea is simple - catch abstract object (which does not have @base attribute) that does not have inner <o> elements

@h1alexbel
Copy link
Member Author

h1alexbel commented Jan 16, 2025

@maxonfjvipon @yegor256 in this snippet:

[] > app
  [] > a1
  if. > @
    true
    []
      2 > @
    1
  [x] > a2

a1 will be reported together with true. The rest of objects won't be reported, as expected. According to the XMIR, true
is indeed empty:

<o line="2" name="app" pos="0">
  <o line="3" name="a1" pos="2"/>
  <o base=".if" line="4" name="@" pos="2">
    <o base="true" line="5" pos="4"/>
    <o line="6" pos="4">
      <o base="number" line="7" name="@" pos="6">40-00-00-00-00-00-00-00</o>
    </o>
    <o base="number" line="8" pos="4">3F-F0-00-00-00-00-00-00</o>
  </o>
  <o line="9" name="a2" pos="2">
    <o base="" line="9" name="x" pos="3"/>
  </o>
</o>

Here it is:

<o base="true" line="5" pos="4"/>

WDYT?

@yegor256
Copy link
Member

@h1alexbel true has @base, that's why not relevant to this lint

@h1alexbel
Copy link
Member Author

@maxonfjvipon updated. Take a look, please

@h1alexbel
Copy link
Member Author

@maxonfjvipon reminder

1 similar comment
@h1alexbel
Copy link
Member Author

@maxonfjvipon reminder

@maxonfjvipon
Copy link
Member

maxonfjvipon commented Jan 20, 2025

@h1alexbel did you see this? It seems that condition /program/objects//o[not(@base) and not(o) and not(@atom)] is just enough without any voids checking

@h1alexbel
Copy link
Member Author

@maxonfjvipon yes, now I got this. Updated PR, take a look, please

@h1alexbel
Copy link
Member Author

@maxonfjvipon reminder

<xsl:text> is empty. It doesn't have any attributes, neither void nor attached</xsl:text>
</xsl:element>
<xsl:for-each select="/program/objects//o[not(@base) and not(o) and not(@atom)]">
<xsl:if test="not($void = @base)">
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel this condition is not necessary anymore, this condition in for-each /program/objects//o[not(@base) and not(o) and not(@atom)] is enough to find empty objects, also variable $void may be deleted

@maxonfjvipon
Copy link
Member

@h1alexbel check above, I believe this is the last one and we're good to merge

@h1alexbel
Copy link
Member Author

@maxonfjvipon updated, please check once again (ort fails in master branch)

@h1alexbel
Copy link
Member Author

@yegor256 take a look, please

@yegor256 yegor256 merged commit 5bd8e7f into objectionary:master Jan 22, 2025
14 of 15 checks passed
@yegor256
Copy link
Member

@h1alexbel thanks!

@h1alexbel h1alexbel deleted the 221 branch January 22, 2025 07:40
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