[CLJ-852] IllegalArgumentException thrown when defining a var whose value is calculated with a primitive fn. Created: 10/Oct/11 Updated: 23/Mar/12 Resolved: 23/Mar/12
|Affects Version/s:||Release 1.3|
|Fix Version/s:||Release 1.4|
|Reporter:||Alexander Taggart||Assignee:||Ben Smith-Mannschott|
|Attachments:||CLJ-852-fix.patch clj-852-patch2.txt CLJ-852-rfc-fix.patch CLJ-852-test.patch|
|Patch:||Code and Test|
Note the following stacktrace line numbers are from the latest 1.4-snapshot, though this happens on 1.3 as well.
Note though that the following works fine:
|Comment by Ben Smith-Mannschott [ 15/Oct/11 10:13 AM ]|
|Comment by Ben Smith-Mannschott [ 15/Oct/11 10:41 AM ]|
|Comment by Ben Smith-Mannschott [ 15/Oct/11 2:59 PM ]|
add special cases for "int"…"boolean" to tagToClass()
This causes the test to pass, however, there are still some open questions:
Why does tagToClass first call maybeClass and then possibly overwrite the result it gets back without even looking at it? This strikes me as surprising. I would have expected a check for null, and then only running the if/else chain of special cases if maybeClass failed to return a non-null result.
The suppressed exception in maybeClass commented with "//aargh" makes me wonder if maybeClass and tagToClass are factored correctly.
|Comment by Stuart Halloway [ 25/Oct/11 6:00 PM ]|
This looks reasonable to me, but given the number of places tagToClass gets called I would definitely want Rich to take a look.
|Comment by Ben Smith-Mannschott [ 02/Nov/11 6:12 AM ]|
|Comment by Ben Smith-Mannschott [ 13/Nov/11 9:18 AM ]|
This fixes the issue. It appears to me like the cases "int" .. "boolean" (i.e. the non-array primitive types) were simply forgotten in tagToClass().
It's not impossible that tagToClass is being misused somehow ant that's the cause of the problem, but I see no evidence for that. (I'd need a better understanding of the code base to make that determination – mind reading skills would help too.)
It's also conceivable that maybeClass() should be doing the resolution from primitive type to Class object, but that doesn't seem likely to me.
|Comment by Andy Fingerhut [ 22/Feb/12 9:02 PM ]|
clj-852-patch2.txt is exactly the same as Ben's
(1) I moved his new unit tests to an already-existing file metadata.clj, which seemed to have related tests with def and metadata, whereas Ben's patch adds a brand new test file. Not sure whether keeping the number of files down is important or not.
(2) I removed some non-ASCII characters from his commit notes.
Both his patch and this newer one apply cleanly to latest master as of Feb 22, 2012. Neither adds any new errors or warnings to compilation or tests (unless you apply the new test part of the patch without his proposed compiler fix, where the new test does fail as expected). No docstrings need changing that I can think of. Ben and I have both signed CAs. Patch status is "Code and Test".
|Comment by Stuart Sierra [ 24/Feb/12 1:32 PM ]|
Screened. The addition seems harmless, merely recognizing more tags in tagToClass.