ReadOnlyPrimitives hint in codechecks not firing

[grizeldi]

I was working on #37 when I figured out that ReadOnlyPrimitivesHint does not work. As far as I understand it it's purpose is to prevent people from using stuff like node.getLocalTranslation.set(/*Whatever*/); I tried it on my fork. Didn't work. Tried it on this repo. Didn't work. Now I could go fix it myself, but ATM I'm still moving strings to bundles.

[MeFisto94]

Hey, The thing why I didn't comment on the other issue or something is I actually don't have a clue about this and also not really the time to depply dig into that. node.getLocalTranslation().set() itself is actually a pretty valid code.

Again: Without knowing the interna, I imagine ReadOnlyPrimitives rather as class fields of types like float, int, bool, etc.

Actually Vector3f isn't a primitive and readOnly rather means stuff like Vector3f.ZERO or comparable.

But then I just looked in the code: See if you can trigger it by using Vector3f.ZERO.set(abc); since this is what we always tried to detect (especially in such cases as outlined in the code snippet below).

@normen Could it be that we wanted to implement this feature but never completely did that since we wanted to go down that Annotation Path?

Node n = new Node("Test");
n.setLocalTranslation(Vector3f.ZERO);
n.getLocalTranslation().set(Vector3f.UNIT_Y); // this "destroys" Vector3f.ZERO

[pspeed42]

"node.getLocalTranslation().set() itself is actually a pretty valid code."

...only if you want to confuse the scene graph. If you do that, then you still end up having to call setLocalTranslation() right after or your update won't take. Better to just not do it.

I don't know if that's what the annotation pertains to but try to avoid that code anyway.

On Mon, Apr 4, 2016 at 4:46 PM, MeFisto94 notifications@github.com wrote:

Hey, The thing why I didn't comment on the other issue or something is I actually don't have a clue about this and also not really the time to depply dig into that. node.getLocalTranslation().set() itself is actually a pretty valid code.

Again: Without knowing the interna, I imagine ReadOnlyPrimitives rather as class fields of types like float, int, bool, etc.

Actually Vector3f isn't a primitive and readOnly rather means stuff like Vector3f.ZERO or comparable.

But then I just looked in the code: See if you can trigger it by using Vector3f.ZERO.set(abc); since this is what we always tried to detect (especially in such cases as outlined in the code snippet below).

@normen https://github.com/normen Could it be that we wanted to implement this feature but never completely did that since we wanted to go down that Annotation Path?

Node n = new Node("Test"); n.setLocalTranslation(Vector3f.ZERO); n.getLocalTranslation().set(Vector3f.UNIT_Y); // this "destroys" Vector3f.ZERO

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/jMonkeyEngine/sdk/issues/49#issuecomment-205488428

[grizeldi]

Tested it. Not working. This is officially a bug. However I don't know if we should fix it since I will be migrating everything to the annotation based API soon?

[tonihele]

The thing is that the hint looks for usages of the annotation ReadOnly, on method calls. These are (no longer?) not used in the engine itself. Of course they can be used by 3rd party code, but there is the challenge that we wont know what for.

Same actually goes with the other hint, the InternalMethod hint. The Internal annotation is also not used in the engine code, well, not in a method currently. So it does nothing.

I also believe that it was meant to protect the Vector3f.ZERO and the likes. That would awesome if it would, but really super hard to detect. Since you can assign the constant to million different places and modify it with wide variety of different methods. I'm not sure the the code check is smart enough to give us this information that you are messing with the constant. If it is smart enough, we need to hard code the constants we want to protect since they are not using the annotation I believe they were supposed to wield.

Anyway, https://github.com/jMonkeyEngine/sdk/pull/221 is just straight conversion to the newer Hint framework. I did see that these aren't really working as intended. But I saw the value of having them converted anyway for historical purposes, they can be later deleted or modified.

[normen]

These annotations were meant to be used at compile time in an annotation processor attached to the compiler but apart from a skeleton for the processor I posted on googlecode it was never realized fully so the only real use was in the SDK.