UMLR — Problems with OCL definition of Package::makesVisible

Looking at the UML2.5 specification draft (I have the document UML25AfterBallot9.pdf - not sure if this is the newest one)
I see problems with definition of Package::makesVisible - which is expressed in OCL.:

makesVisible(el : NamedElement) : Boolean
The query makesVisible() defines whether a Package makes an element visible outside itself. Elements with no visibility and elements with public visibility are made visible.
pre: member->includes(el)
body: ownedMember->includes(el) or
(elementImport->select(ei|ei.importedElement = VisibilityKind::public)>collect(importedElement.oclAsType(NamedElement))>includes(el)) or
(packageImport->select(visibility = VisibilityKind::public)>collect(importedPackage.member>includes(el))->notEmpty())

Actually those problems carry on from the previous versions of UML;
but since in previous versions even the OCL syntax was wrong (carried over from the pre-OCL2.0 times)
I assumed this section is old/abandoned and did not pay much attention to it.

But now with UML2.5 somebody took it seriously to update the syntax of the OCLs (kudos for that brave soul ), so we have an updated variant.
But while the raw syntax problems were fixed, semantic problems were carried form the old revision verbatim.
If we are updating OCLs anyway, I think it would be a good time to also correct those.

So here goes:

--------------------------------------------------------------------------------

Problem #1

the following comparison is nonsensical (the case handling ElementImports, line #2 of the body):

ei.importedElement = VisibilityKind::public

The OCL here tries to compare the model element (at the end of ElementImport relationship) with the enumeration literal - VisibilityKind::public, which is not what we want
I think this passage should be restated as follows:

ei.visibility= VisibilityKind::public

i.e. we want to test whether element import has visibility set to public, just as in the other case - with package imports - one line below.

Also the whole case handling element imports could be rewritten to simplify it:
elementImport->exists(ei|ei.visibility = VisibilityKind::public and ei.importedElement = el)
This does not change the semantics, but is much better readable/understandable: we are iterating through all (public) element imports
checking whether imported element matches the element el.

--------------------------------------------------------------------------------

Problem #2
the case handling package imports (line #3 of the body) is also borked:

packageImport->select(visibility = VisibilityKind::public)>collect(importedPackage.member>includes(el))->notEmpty()

Here the first part of the expression is OK; we take all package import relationships and filter them - accept only public ones:

packageImport->select(visibility = VisibilityKind::public)

But the next part again makes no sense

...>collect(importedPackage.member>includes(el))->notEmpty()

here expression part

importedPackage.member->includes(el)

produces a boolean - whether element el is included among the members of the package being imported.
So the result of the expression part

...>collect(importedPackage.member>includes(el))...

is a collection of booleans (of the form:

),
where each boolean signifies whether element is among the members of each particular imported package.

Then it makes no sense to test that for emptiness:

->notEmpty()

this produces true if there is at least one item (does not matter true, or false) in that bag of booleans.
So that part produces true if there is at least 1 public package import ( it does not matter what package is imported).

I think this passage should be restated as follows:

packageImport->select(visibility = VisibilityKind::public)>exists(importedPackage.member>includes(el))

I.e. we are iterating through all (public) package imports and checking whether element el appears among members
of at least one of the imported packages.

--------------------------------------------------------------------------------

So the final OCL of makesVisible could be (also getting rid of some unnecessary parentheses, and further simplification):

pre: member->includes(el)
body:
ownedMember->includes(el) or
elementImport->exists(ei|ei.visibility = VisibilityKind::public and ei.importedElement = el) or
packageImport->exists(pi|pi.visibility = VisibilityKind::public and pi.importedPackage.member->includes(el))