Community
Participate
Working Groups
A safe navigation is giving me an unexpected null source navigation failure (In both, interpreted and CGed execution). If I replace the following expression: ownedCallExp = ownedNameExp?.ast() by ownedCallExp = if ownedNameExp = null then null else ownedNameExp.ast() endif The execution doesn't fail Repro coming next (as soon as I have the bug id).
Branches: OCL -> asanchez/485668 QVTd -> asanchez/485668 Repro: - Execute OCL2QVTiTestCases - testNewExample2_V2_CG and testNewExample2_V2_Interpreted fail The rework described in comment 0, pushed as asanchez/485668ReworkToPass to demonstrate success. I've realized that it's a CS2AS issue, because the safe navigation is lost during the generated qvtp.qvtias. Moving to proper project and assigning to me
Should be a trivial feature copy across.
Not so trivial, because I would have expected that feature copy, because EcoreUtil.copy is used at OCL expressions level. Investigating...
(In reply to comment #3) > Not so trivial, because I would have expected that feature copy, because > EcoreUtil.copy is used at OCL expressions level. Investigating... I forgot the special treatment of ast OpCallExps which are rewritten as ProperatyCallExps, and this Safe Navigation are one of those. You were right. Unfortunately, after fixing that, there is still an unexpected null-source access. It seems an scheduler issue. Whatever you have considered in the scheduler to do with if-then-else-exps, safe call exps should have similar considerations. I'll let you go on with the resolution of this issue. Branches: OCL -> asanchez/485668 QVTd -> asanchez/485668 I've also found an issue with the AS2CS, for which I've also got a fix. Ill raise a separate bug.
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #4) > It seems an scheduler issue. Whatever you have considered in the scheduler > to do with if-then-else-exps, safe call exps should have similar > considerations. The code for isSafe is missing in the QVTp2QVTs conversion. ? expand it ? copy it Expand may offer some merge opportunities.
But it will help to have valid input. CS2AS is translating the *.ocl classescs2asV2.ocl: ownedCallExp = ownedNameExp?.ast(), -- FIXME, this is not properly CGed. See PropertyCallExp creation to classescs2asV2.qvtp.qvtias: nameExpCS.ast.oclAsType(as::OperationCallExp).ownedCallExp := nameExpCS.ownedNameExp.ast?.oclAsType(as::CallExp); which is making the wrong access safe.
(In reply to Ed Willink from comment #6) > But it will help to have valid input. CS2AS is translating the *.ocl > .... > which is making the wrong access safe. Ah! probably using the old buggy tooling.
(In reply to comment #7) > > Ah! probably using the old buggy tooling. Indeed. (In reply to comment #4) > > I've also found an issue with the AS2CS, for which I've also got a fix. Ill > raise a separate bug. Whereas asanchez/485668 includes the fix to produce the expected safe navigation in the generated qvtp.qvtias, you need the fix of Bug 485696 to properly visualize the safe navigation in the textual qvti editor.
(In reply to Ed Willink from comment #5) > The code for isSafe is missing in the QVTp2QVTs conversion. It is awkward handling isSafe during QVTp2QVTs since there are multiple code paths affecting property navigations within predicates / computations. Much easier to add QVTbaseUtil.rewriteSafeNavigations and call it on the pModel before the domain analysis takes place. (Better to put it in the xxx2QVTp generators, but we have multiple xxx2QVTp generators at present.)
(In reply to Ed Willink from comment #9) > Much easier to add QVTbaseUtil.rewriteSafeNavigations Once the scheduler is upgrded to handle the non-navigable variable for the if test, the problem moved back to CS2AS. ownedCallExp = ownedNameExp?.ast() is enhanced to ownedCallExp = ownedNameExp?.ast().oclAsType(...) If the source of the oclAsType is !isRequired, then the oclAsType must also be isSafe.
(In reply to Ed Willink from comment #10) > (In reply to Ed Willink from comment #9) > > Much easier to add QVTbaseUtil.rewriteSafeNavigations > > Once the scheduler is upgrded to handle the non-navigable variable for the > if test, the problem moved back to CS2AS. > > ownedCallExp = ownedNameExp?.ast() > > is enhanced to > > ownedCallExp = ownedNameExp?.ast().oclAsType(...) > > If the source of the oclAsType is !isRequired, then the oclAsType must also > be isSafe. I presume that a isSafe oclAsType might produce a premature computation of ownedCallExp (ownedNameExp has not produced the AS element yet) If that helped, I might make the ast() ops to mandate a result, where appropriate. Did you try that ?
I don;t think you follow. Safe navigation does not solve problems; it just pushes them sideways. Originally we had in QVTp: ownedCallExp = ownedNameExp.ast().oclAsType(...) But ownedNameExp may be null, so the safe navigation tolerates it ownedCallExp = ownedNameExp?.ast().oclAsType(...) But now the source of oclAsType() can be null, so we need ownedCallExp = ownedNameExp?.ast()?.oclAsType(...) NB unlike "null instanceof X" in Java, null.oclAsType(X) is not valid in OCL.
(In reply to Ed Willink from comment #12) > I don;t think you follow. Safe navigation does not solve problems; it just > pushes them sideways. If I say that your are saying the same in your new exposition with respect to the previous one, am I following or not ?.... sigh Forget about my last suggestion. The workaround of using if-then-else-endif, made the test case work. So we don't really need neither a safe navigation over oclAsType source, nor changing to ast() ops to be non-null. Just need to re-check that asanchez/485668ReworkToPass still works with the more recent state of the code to provide some evidences. I'll give it a try tomorrow.... tired for today. Regards, Adolfo.
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #13) > Just need to re-check that asanchez/485668ReworkToPass still works with the > more recent state of the code to provide some evidences. I'll give it a try > tomorrow.... tired for today. I saw this morning you brought that specific commit to your branch. So I presume you have verified my suspicions. It seems you are on this, so no need to reproduce anything from my side. If there is anything that OCL2QVTp can improve to sort the issue, let me know. Regards, Adolfo.
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #14) > It seems you are on this The branch is ready to push to master as soon as HIPP6 recovers and allows a branch tests. > If there is anything that OCL2QVTp can improve to sort the issue, let me know. The missing ?.oclAsType() is a low priority bug, waiting to bite as soon as the source OCL gets more elaborate. (Or when we pay attention to the many OCL validation warnings.) It's probably only a line in the ETL. On the other hand if you migrate to Java, it could be 0 lines, since it could be buried in MetamodelManager.createOperationCallExp().
(In reply to Ed Willink from comment #15) > (In reply to Adolfo Sanchez-Barbudo Herrera from comment #14) > > It seems you are on this > > The branch is ready to push to master as soon as HIPP6 recovers and allows a > branch tests. If the branch simply contains the workaround to make the test pass, you are not solving the bug but using the workaround I could use at first place. > > The missing ?.oclAsType() is a low priority bug, waiting to bite as soon as > the source OCL gets more elaborate. (Or when we pay attention to the many > OCL validation warnings.) So it seems we still have a distinct view of where the problem is. You say there is an ?.oclAsType missing. I pointed out that there was a workaround, which exposes how the if-then-else-end expression (equivalent to the safeNavigation) with the *missing* ?.oclAsType made the test case pass. So I'm not convinced yet that generating that ?.oclAsType is a solution of the bug. At most, another workaround. When you say that "the ?.oclAsType is needed", it's -probably- because the expression is prematurely executed. And it's -surely- prematurely executed because of a bad scheduler. On the contrary, the scheduler is doing well under an IfExp presence, because the workaround of asanchez/485668ReworkToPass makes the test case pass. I recall my original thought: (In reply to Adolfo Sanchez-Barbudo Herrera from comment #4) > It seems an scheduler issue. Whatever you have considered in the scheduler > to do with if-then-else-exps, safe call exps should have similar > considerations. Regards, Adolfo.
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #16) You miss the problem. What I think is happening is: The ast property had an abstract type so when the ast() gives you a stronger type you enforce it by appending .oclAsType(StrongerType). However if it was ?.ast then ?.ast.oclAsType(StrongerType) is unsafe OCL. The scheduler now correctly waits for the ast value, which may be null, to be determined then executes oclAsType and fails if it is null. When appending you must replicate the safe navigation so if it is x?.ast then it must extend to x?.ast?.oclAsType(...)
I don't think you follow... Changing approach, because we are stuck... I've got your ewillink/485668 and executed test cases and all pass. Good. Now I explore the classescs2asV2.qvtp.qvtias and focus on uOperationCallExp_ownedCallExp mapping At line 199, I see the following expression in the else part: nameExpCS.ownedNameExp.ast.oclAsType(as::CallExp) I clearly identify two potential null navigations: a) at .ast -> We could (/should?) have a safe navigation here (i.e ?.ast). However, I think it's kinda stupid since the 'if' condition should ensure that the source is not null. b) at .oclAsType(as::CallExp) -> We could (/should?) have a safe navigation here (i.e ?.oclAsType(as::CallExp)). You say that b) is *needed*. However the example doesn't have the safe navigation on ?.oclAsType and the test case pass. If b) were really needed, could you explain me how the test could ever pass ?
By putting in the explicit if, you have avoided the oclAsType being applied to null; the oclAsType is appended within the else condition. So your workaround is a genuine fix but requires that save navigation is explicit in any CS2AS OCL.
(In reply to comment #19) > By putting in the explicit if, you have avoided the oclAsType being applied to > null; the oclAsType is appended within the else condition. This doesn't make sense. Within the else part you can only ensure that nameExpCS.ownedNameExp IS-NOT null. Therefore, indeed a) is not necessary. However, nobody can ensure that nameExpCS.ownedNameExp.ast IS NOT NULL. Therefore oclAsType might be applied to null. A clever scheduler might avoid the .oclAsType null access, which seems to occur with if-then-else-endif expressions, but apparently, it's not doing the same clever considerations with the equivalent safe navigations.
(In reply to comment #19) > > So your workaround is a genuine fix but requires that save navigation is > explicit in any CS2AS OCL. Sorry but since both expressions (safe navigation and if-then-else-endif) are equivalent, I can't accept that the workaround can be the fix. We can debate/argue if the the ?.oclAsType is convenient, I might even agree that it should be generated (this bring me other concerns I haven't wanted to come across), but there is something smelly regarding to safe-navigation from the point of view of the scheduler, which we should not simply pass. Who knows, how many unsafe navigations might appear in potential transformations, that a scheduler might properly overcome. NB. You might find lots of unsafe navigations (e.g missing ?.oclAsType) along the generated qvtp transformations, and this is not currently preventing the scheduler to do good work. We shouldn't reduce the issue to a missing ?.oclAsType safe navigation.
With the fix/workaround the serialized.qvti is: _'=' : Boolean := ownedNameExp = null, ownedCallExp : classes::CallExp := if _'=' then null else ownedNameExp.ast.oclAsType(classes::CallExp) endif giving two distinct computations "null" and "ownedNameExp.ast.oclAsType(classes::CallExp)" according to whether "ownedNameExp" is null or not. Without the fix/woaround serialized.qvti is: aCallExp : classes::CallExp := if _'=' then null else ownedNameExp.ast endif.oclAsType(classes::CallExp) In both cases OCL promises to the executor that the source of .oclAsType() will be non null. Why should the scheduler do something other than what OCL expects?
(In reply to Ed Willink from comment #22) > With the fix/workaround the serialized.qvti is: > > _'=' : Boolean := ownedNameExp = null, > ownedCallExp : classes::CallExp := if _'=' > then null > else ownedNameExp.ast.oclAsType(classes::CallExp) endif > > giving two distinct computations "null" and > "ownedNameExp.ast.oclAsType(classes::CallExp)" according to whether > "ownedNameExp" is null or not. > > Without the fix/woaround serialized.qvti is: > > aCallExp : classes::CallExp := if _'=' > then null > else ownedNameExp.ast > endif.oclAsType(classes::CallExp) > > In both cases OCL promises to the executor that the source of .oclAsType() > will be non null. Why should the scheduler do something other than what OCL > expects? You mention a rewrite at comment 9. May be, that's the problem. The rewrite seems incorrect, because you are following the navigation even though in the case when you ownedNameExp is null. The oclAsType should be in the else part, rather than following the IfExp
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #24) > The oclAsType should be in the else part, rather than following the IfExp Yes, if we want it to work. But that is not the definition of safe navigation. "a.b.c" requires "a" and "a.b" to be non-null to avoid an invalid execution. "a?.b.c" makes the "a" navigation safe but still requires "a?.b" to be non-null to avoid an invalid execution. (Hence I describe safe navigation as just pushing the problem sideways.) It is (let aDash = a in if aDash == null then null else aDash.b endif).c Only "a?.b?.c" is fully safe internally. (let abDash = (let aDash = a in if aDash == null then null else aDash.b endif) in if abDash == null then null else abDash.c endif) "a?.b.c" is, I think, already a warning. Perhaps it should be an error. So when you synthesize "x.?ast.oclAsType(...)" you should synthesize either the workaround: "if x = null then null else x.ast.oclAsType(...) endif" or "x.?ast?.oclAsType(...)" using your knowledge that, when read at the correct time, the ast property can never be null. This knowledge should be in the metamodel so the CG should at some point exploit it anyway and so avoid the double if.
(In reply to comment #25) > (In reply to Adolfo Sanchez-Barbudo Herrera from comment #24) > > The oclAsType should be in the else part, rather than following the IfExp > > Yes, if we want it to work. But that is not the definition of safe navigation. > Wow. Don't you see that when ownedNameExp = null, you are doing ownedNameExp.oclAsType(classes::CallExp) ??? Which is not just a null access, but a completely different expression ? I strongly suggest you to do a step back... Fortunately we are not inventing safe navigation. For instance, Xtend have some comments on it [1] a?.b should mean if (a == null) then null else a.b endif --- a?.b.c should mean if (a == null) then null else a.b.c endif your rewrite is producing if (a == null) then null else a.b endif.c Which I hope you see now that is wrong. [1] https://eclipse.org/xtend/documentation/203_xtend_expressions.html#null-safe-feature-calls
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #26) Your Xtend reference is irrelevant. For a.b there is no difference. The problem arises with a.b.c which in OCL is (a.b).c hence a?.b.c is (a?.b).c. This is indeed almost certainly useless since a?.b.oclIsUndefined() is one of very few possible examples where it could be meaningful. a?.b->size() is of course fine. We can add validation warnings/errors, but I think a non-local syntax sugar expansion of a?.b is undesirable.
(In reply to Ed Willink from comment #27) > (In reply to Adolfo Sanchez-Barbudo Herrera from comment #26) > Your Xtend reference is irrelevant. For a.b there is no difference. Are you saying that resolving exp1?.exp2 should be different depending on how simple or complex exp1 and exp2 looks like ? > The problem arises with a.b.c which in OCL is (a.b).c hence a?.b.c is > (a?.b).c. > Which problem ? The current problem is the wrong rewrite you are producing, and you are not able to see/admit it. A safe navigation in a programming language should be the same regardless of how the AS actually looks like. As safe navigation such as : a?.what-ever-you-want-to-put-here should be interpreted as: as soon as a == null, you should not navigate any further (regardless of how complex/long is the expression you have after the safe operator) and simply return null. Your rewrite is doing something horribly different. Maybe you want to invent a new concept of safe navigation. Even after trying hard to understand what you want to bring by introducing those parenthesis in the expression. I understand it, but it doesn't make sense: providing that navigating .c after performing a?.b made sense, that would force you to ALWAYS use safe navigators every time you use them the first time (regardless navigating b.c were safe or not). > > We can add validation warnings/errors, but I think a non-local syntax sugar > expansion of a?.b is undesirable. I'm not fond of transforming a safe navigations into if-then-else-endif at all (what about traceability?). What is clear to me is that the current expansion is simply wrong. Ideally, the scheduler/execution engine should be able to deal with these safe navigation concepts transparently.
Ref: https://en.wikipedia.org/wiki/Operator_associativity You don't seem to understand the concept of left/right associativity, which allows "a £ b £ c" to be written unambiguously for a non-associative operator £. The specified associativity determines whether "a £ b £ c" is to be interpreted as "(a £ b) £ c" or "a £ (b £ c)". The OCL "." navigator is left-associative, just the same as I think all other OO languages. So "a.b.c" is to be treated as "(a.b).c" therefore the naive syntax sugar expansion of "a?.b.c" must be treated as "(a?.b).c" It is simple and regular and its limitations are easily diagnosed by competent validation rules. I am far from clear what alternative you are suggesting other than one that suits a specific example.
I understand them, but I am afraid you are bringing a red herring. [I'm gonna try a last attempt] operands associativity is a parsing problem (it's needed to obtain the right AS). Here, there is no parsing problem at all . We are discussing an evaluation problem related to null safe navigation expressions, that you have tried to overcome by rewriting them as If-then-else-endif expressions (your piece of code of comment 22 was very useful to figure this out), which might be an acceptable solution but you have done it a wrong way (third time I mention?). Probably due to a misunderstanding of what a safe navigation is, and the real motivation for them. Xtend explains a bit of it, you might want to google about it and you will find more examples in other languages. The motivation is quite simple (you even wrote a paper about this:\): If I have an expression such as a.b.c.d....z I'm exposing two different cases: a) where all access might be null A null safe code (without safe navigations) would be something like the following: Object result; if (a!=null && a.b !=null && a.b.c != null && a.b.c.d != null && ... a.b.c.d...y != null ) { result = a.b.c.d...y.z; } else { result = null } b) where only the first access might be null A null safe code (without safe navigations) would be something like the following: if (a != null) { result = a.b.c.d...z; } else { result = null; } Because the code is less readable (specially in a)) null safe navigators come to the rescue a) a?.b?.c?.d?...?.z b) a?.b.c.d....z In both cases the evaluation of such safe navigations are the same, whenever you find a safe navigation, if the source is null, don't go on evaluating further, just return a null. It's a short circuit[1] It's exactly the same evaluation strategy we find when dealing with condition short circuits. Whenever you evaluate the left operand of and &&, if you already get a false, don't go on evaluating further, just go for the else-part evaluation. --------------- Our example resembles to b) However you are doing the rewrite wrong (I need to repeat it). Providing that I rewrite the b) if-expression I wrote before as the following equivalent one: Object result; if (a == null) { result = null } else { result = a.b.c.d...z; } What your wrong rewrite is doing is the following: if (a == null) { result = null.b.c.d...z; } else { result = a.b.c.d...z; } More specifically (in OCL terms): if a == null then null else a.b endif.c.d....z; [1] http://stackoverflow.com/questions/8759868/java-logical-operator-short-circuiting
In Xtend give var String sNull = null; var String a = sNull ?. toString() . toString(); a try. NPE; just the same as the OCL implementation.
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #30) > Because the code is less readable (specially in a)) null safe navigators > come to the rescue > > a) a?.b?.c?.d?...?.z > b) a?.b.c.d....z > > In both cases the evaluation of such safe navigations are the same, whenever > you find a safe navigation, if the source is null, don't go on evaluating > further, just return a null. It's a short circuit[1] > > It's exactly the same evaluation strategy we find when dealing with > condition short circuits. Whenever you evaluate the left operand of and &&, > if you already get a false, don't go on evaluating further, just go for the > else-part evaluation. No. OCL does not specify short circuit evaluation. It doesn't need it since expressions are side effect free. Instead it just has the non-strict rules that e.g false and anything is false. Equally anything and false is false. "The rules for OR and AND are valid irrespective of the order of the arguments and they are valid whether the value of the other sub-expression is known or not." AND and OR are associative so any evaluation order is possible. Introducing short circuit evaluation requires them to be right-associative. a && (b && c).
(In reply to Ed Willink from comment #32) > (In reply to Adolfo Sanchez-Barbudo Herrera from comment #30) > > Because the code is less readable (specially in a)) null safe navigators > > come to the rescue > > > > a) a?.b?.c?.d?...?.z > > b) a?.b.c.d....z > > > > In both cases the evaluation of such safe navigations are the same, whenever > > you find a safe navigation, if the source is null, don't go on evaluating > > further, just return a null. It's a short circuit[1] > > > > It's exactly the same evaluation strategy we find when dealing with > > condition short circuits. Whenever you evaluate the left operand of and &&, > > if you already get a false, don't go on evaluating further, just go for the > > else-part evaluation. > > No. OCL does not specify short circuit evaluation. It doesn't need it since > expressions are side effect free. Instead it just has the non-strict rules > that e.g false and anything is false. Equally anything and false is false. It's not a matter of need. It's a matter of speed up. OR/AND are just library operations in OCL, so I agree that it doesn't/can't implement short circuit evaluation. Other languages, might do.
(In reply to Adolfo Sanchez-Barbudo Herrera from comment #33) > > No. OCL does not specify short circuit evaluation. It doesn't need it since > > expressions are side effect free. Instead it just has the non-strict rules > > that e.g false and anything is false. Equally anything and false is false. > > It's not a matter of need. It's a matter of speed up. OR/AND are just > library operations in OCL, so I agree that it doesn't/can't implement short > circuit evaluation. Other languages, might do. OCL does not need to specify short circuit evaluation. An implementation most certainly should try to provide it. The failure of the CGed implementation to skip executing the second half of an implies expression is a major nuisance when debugging; tons of NPEs and invalids. While the nuisance is hidden when not debugging, it is clearly a waste of computational effort. I'm hoping that once implies etc are inlined, the exposure of the true semantics will allow the problem optimize away.
(In reply to Ed Willink from comment #31) > In Xtend give > > var String sNull = null; > var String a = sNull ?. toString() . toString(); > > a try. NPE; just the same as the OCL implementation. Surprised by this, so I'm not going to push you further on my beliefs. I presume you understand my concern, so it's up to decide you what is better for OCL, and we have already spent enough time on this. I still believe it's a bad idea (not applying the short circuit evaluation in OCL to the whole trailing expression), and therefore: ?a.b.c should be always valid if 'b' mandates a value and should mean: if a = null then null else a.b.c endif With current OCL behaviour, as soon as you use the safe navigator once, you are mandated to use it for every further navigation. This is even worse in OCL, since you know about what might be and can't be null. So, having to use a safe navigator when the source is supposed to mandatorily have a value, is confusing. [NB Validation rules will not only have to take into account meta-model information, but also previous (ownedSource) navigations, just in case they were safe] Even though the "ast" traceability property were mandatory for every ElementCS, my OCL2QVTp tx would have to take into account if the source of the navigation was safe or not, in order to produce that ?.oclAsType or not. [NB Since I'm/we are currently modelling that traceability property as optional, I guess I can afford to always produce the safe ?.oclAsType navigation. Bewaring on that further navigations from ?.oclAsType are safe as well. phew !] I feel all of this unnecessary, but if you stick to this (IMHO) poor safe navigation behaviour, I will agree on going for it. Regards, Adolfo.
(In reply to comment #35) > > Surprised by this, so I'm not going to push you further on my beliefs. I presume > you understand my concern, so it's up to decide you what is better for OCL, and > we have already spent enough time on this. > > I still believe it's a bad idea (not applying the short circuit evaluation in > OCL to the whole trailing expression), and therefore: Tried another (non-modeling, though) language such as groovy. Same behaviour. Regardless of possible confusions, anybody used to them should not be surprised of having to trail many of them (regardless you are navigating optional or mandatory properties).
(In reply to comment #35) > Even though the "ast" traceability property were mandatory for every ElementCS, > my OCL2QVTp tx would have to take into account if the source of the navigation > was safe or not, in order to produce that ?.oclAsType or not. Done in asanchez/master. The examples safe navigations have been recovered commit 974336f5cf3cfbd2291328d438fc65d41054b912 ([485668] - Fixing the OCL2QVTp transformation to spread the safe navigation to oclAsType call)
