Configurable checks for GWT

[Goktug Gokdogan]

I have a patch that will introduce a preconditions helper class to be used internally by the SDK. It will be very similar to the one in Guava with some modifications.

My goals with this class are:

 1- Ability to provide different levels of checking (e.g. development vs production) without worrying about code size or performance.

 2- Improve code readability across the SDK.

Here is the quick design doc:

https://docs.google.com/document/d/1AegOijkqg9ix6wtMMPYU0JeDBZXIzSgopDifPV7gH9E/edit?usp=sharing

Please feel free to comment. I'll be on vacation next week and will probably respond the other week.

Thanks,

- Goktug

[Jens]

Sometimes I am not sure if we should use a normal or a critical check.

Basically my understanding is that we should use a normal check if the code would also fail (but obviously with a JS error instead of a required Java Exception) with an error if the check was not present. If the code would fail pretty late and debugging will be difficult we better use a critical check instead to help debugging. However if the code will not fail if a check has been compiled out then we should always use a critical check, especially if JavaDoc requires the code to fail. Is that correct?

Here are some examples I struggle with:

1.) checkNotNull() if JavaDoc requires NPE to be thrown in all cases.

void forEach(Consumer c){
  checkNotNull(c);
  for (T item : items) {
    c.accept(item);
  }
}

JavaDoc says NPE must be thrown if consumer is null. If checks are compiled out, for the special case that items is empty it would not fail with a normal check, so we should use a criticalCheckNotNull() right?

2.) checking position indexes during index operations on arrays/lists

public static void sort(byte[] array, int fromIndex, int toIndex) {
  checkPositionIndexes(fromIndex, toIndex, array.length);
  nativeNumberSort(array, fromIndex, toIndex);
}

private static native void nativeNumberSort(Object array, int fromIndex, int toIndex) /*-{
  var temp = array.slice(fromIndex, toIndex);
  temp.sort(function(a, b) {
    return a - b;
  });
  var n = toIndex - fromIndex;
  @com.google.gwt.lang.Array::nativeArraycopy(Ljava/lang/Object;ILjava/lang/Object;II)(temp, 0, array, fromIndex, n)
}-*/;

The above is some existing code in GWT. If checks are compiled out nativeNumberSort() could get indexes with fromIndex > toIndex. JavaScript slice() will then return an empty temp array that will be "sorted" and passed on to nativeArracopy() which in turn won't fail as well. So at the end the check should have been a critical check, shouldn't it?

-- J.

[Goktug Gokdogan]

Thanks for asking; I think I never explained this well.

This is mostly judgement call but in general the rule of thumb is:

 - Assume no checks as the starting point

 - Look at the code and see what will happen if we don't have the check:

 - If the missing check will leave the object in a very broken state or will cause other hard to debug issues, add a critical check.

 - Otherwise, add a regular check.

 - The exception to the rule; adding NPE in cases where it would throw TypeError anyway doesn't worth the extra check; and not even needed for J2CL anyway since it will convert them to NPEs.

Keep in mind that; these checks are enabled by default and it is a opt-in for the apps that are willing trade-off some java semantics and some debuggability to get extra performance and reduce code size. So they need to be used sparingly.

If you look at the current examples in JRE, they are only in a handfull places and there is always a good reason to do that.

For example;

 - Throwable.initCause is using it to prevent self-causation as if we don't do that then user can create cycle that might cause infinite loops in code that is processing Throwable; like logging frameworks.

 - Enum does some of the checks with critical; as somebody might use it for verification (and similar for BigInteger).

 - In LinkedHashMap; the check in iterator is critical because otherwise it will leave the iterator in a state that will cause infinite loop.

On Mon, May 9, 2016 at 4:22 AM, Jens <jens.ne...@gmail.com> wrote:

Sometimes I am not sure if we should use a normal or a critical check.

Basically my understanding is that we should use a normal check if the code would also fail (but obviously with a JS error instead of a required Java Exception) with an error if the check was not present. If the code would fail pretty late and debugging will be difficult we better use a critical check instead to help debugging. However if the code will not fail if a check has been compiled out then we should always use a critical check, especially if JavaDoc requires the code to fail. Is that correct?

No actually, meeting JRE contract without much value is a case where we use regular check vs. critical ones.

 

Here are some examples I struggle with:

1.) checkNotNull() if JavaDoc requires NPE to be thrown in all cases.

void forEach(Consumer c){
  checkNotNull(c);
  for (T item : items) {
    c.accept(item);
  }
}

JavaDoc says NPE must be thrown if consumer is null. If checks are compiled out, for the special case that items is empty it would not fail with a normal check, so we should use a criticalCheckNotNull() right?

Doesn't fit to description above; so checkNotNull is appropriate.

 

2.) checking position indexes during index operations on arrays/lists

public static void sort(byte[] array, int fromIndex, int toIndex) {
  checkPositionIndexes(fromIndex, toIndex, array.length);
  nativeNumberSort(array, fromIndex, toIndex);
}

private static native void nativeNumberSort(Object array, int fromIndex, int toIndex) /*-{
  var temp = array.slice(fromIndex, toIndex);
  temp.sort(function(a, b) {
    return a - b;
  });
  var n = toIndex - fromIndex;
  @com.google.gwt.lang.Array::nativeArraycopy(Ljava/lang/Object;ILjava/lang/Object;II)(temp, 0, array, fromIndex, n)
}-*/;

The above is some existing code in GWT. If checks are compiled out nativeNumberSort() could get indexes with fromIndex > toIndex. JavaScript slice() will then return an empty temp array that will be "sorted" and passed on to nativeArracopy() which in turn won't fail as well. So at the end the check should have been a critical check, shouldn't it?

-- J.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/b3bc70c4-be2c-4fd1-a750-c5ada03caeed%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jens]

Hm ok, I think I got it. I would say my Arrays.sort() example should actually use a critical check then because array.slice() can do lots unexpected things (negative indexes for either argument works but results an unexpected subset of array items to be sorted, toIndex can be larger than fromIndex which basically results in no sorting at all) compared to Java. A critical check would make sure that follow up code can expect the array to be sorted they way its meant to be.

I guess in some cases its just tough to draw the line.

-- J.

[Goktug Gokdogan]

On Mon, May 9, 2016 at 12:52 PM, Jens <jens.ne...@gmail.com> wrote:

Hm ok, I think I got it. I would say my Arrays.sort() example should actually use a critical check then because array.slice() can do lots unexpected things (negative indexes for either argument works but results an unexpected subset of array items to be sorted, toIndex can be larger than fromIndex which basically results in no sorting at all) compared to Java. A critical check would make sure that follow up code can expect the array to be sorted they way its meant to be.

That's fair; the way you described actually sounds like a good candidate for checkCritical. We already did similar with Arrays.copy to not end of with messed up indices.

 

I guess in some cases its just tough to draw the line.

Yes, at the end it becomes a judgement call.

 

-- J.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/28e222a3-9138-447c-aeaa-a2daca59ccdd%40googlegroups.com.

[Goktug Gokdogan]

I can see that most of the new usages (e.g. Optional.java) is incorrect. Can you guys fix those?