antlr / stringtemplate4

StringTemplate 4
http://www.stringtemplate.org
Other
955 stars 231 forks source link

Ignore all Exceptions during method and field lookup in ObjectModelAdaptor #250

Closed Clashsoft closed 4 years ago

Clashsoft commented 4 years ago

This addresses #249, and is consistent with getProperty in that it converts any Exception to a STNoSuchPropertyException.

Closes #249

parrt commented 4 years ago

Hmm...I guess it is safe to catch any exception... what could it be ignoring that really should be thrown? is this too broad in other words?

Clashsoft commented 4 years ago

@parrt getMethod can throw NoSuchMethodException and SecurityException, which were caught previously. setAccessible can throw SecurityException in Java 6, but in Java 9 it can also throw InaccessibleObjectException. This lead to a problem in #249. Since we're on Java 6, we cannot refer to and therefore not catch InaccessibleObjectException. The common super type of all of them is Exception

parrt commented 4 years ago

@sharwell Do you see any problems with this PR? basically we will catch and ignore any exception when reading a property

Clashsoft commented 4 years ago

basically we will catch and ignore any exception when reading a property

That pretty much already happens due to this line:

https://github.com/antlr/stringtemplate4/blob/e38a1c81422317d04908c33dfaa5df6c42753653/src/org/stringtemplate/v4/misc/ObjectModelAdaptor.java#L82

It already ignores any exception that occurs even during execution of the getter. This change makes it more consistent in that it now also ignores access-related exception.

Clashsoft commented 4 years ago

Any updates on this? @parrt and @sharwell

parrt commented 4 years ago

@Clashsoft and you've run the unit tests?

Clashsoft commented 4 years ago

Yes, I ran the tests and they were all green :) Btw, the "All checks have passed" at the bottom means that the Travis CI build was successful, which implies that the tests also passed.

parrt commented 4 years ago

haha. I wasn't sure we had Travis connected.