Community
Participate
Working Groups
3.0 m7 assigning to params is a bad smell - but declaring things final is way too much pain for anybody it's be cool if the compiler could detect and report these. e.g. foo(int i){ i = 9; //sign of problems } (in the code above, there's actually also another problem - the param is killed before it's used - another strange smell)
Will reconsider post 3.0
Simple workaround, make your parameters final.
i know but like i said above "declaring things final is way too much pain for anybody" :)
an observation: in cases where the original value of the parameter is never actually used but killed right away with a new value this is even more likely to be a bug.
I personally like reusing parameters as variables to avoid runtime penalty, but this is a valid option to introduce in the compiler (and easy to implement too).
Added a diagnostic for parameter assignment. This diagnostic can be tuned (error, warning, ignore) and is ignored by default. Added a warning external name (for use in -warn:+ or @SuppressWarnings), which is parameter-assignment. The function won't be active until JDT UI surfaces the new warning. See attachments for patches (jdt core plus tests).
Created attachment 26010 [details] Suggested patch for compiler Needs committer review.
Created attachment 26011 [details] Tests
Review: Batch command-line argument should be aligned with existing ones "parameter-assignment" --> "paramAssign" > // REVIEW argument would be more homogeneous with existing code, but JLS puts a > clear emphasis on parameter -- see p.211 for parameter/argument disc. Where did you find that ? We should always denote a parameter using "parameter". > REVIEW the 'accidental' seems extraneous here (see assignment in condition) I would tend to agree, pls file a separate bug for rewording the error msg (and tune the impacted test cases)
Also, the implementation is hooked into flow analysis. Isn't it too late ? We are likely not going to report issues in unreachable code, are we ?
(In reply to comment #9) > > // REVIEW argument would be more homogeneous with existing code, but JLS puts a > > clear emphasis on parameter -- see p.211 for parameter/argument disc. > Where did you find that ? We should always denote a parameter using "parameter". We have (at least): unusedArgument unread method parameter into the messages.properties file for the compiler. Not checked for others.
(In reply to comment #9) > Review: ... > > REVIEW the 'accidental' seems extraneous here (see assignment in condition) > I would tend to agree, pls file a separate bug for rewording the error msg (and > tune the impacted test cases) Entered Bug 117458.
(In reply to comment #10) > Also, the implementation is hooked into flow analysis. Isn't it too late ? We > are likely not going to report issues in unreachable code, are we ? Added test cases to AssignmentTest that show that we do report the issue. I chose the implementation spot by affinity with other assignment diagnostics, within analyseAssignment. Other code there explicitly checks for reachability before raising errors, which I did not do for the parameter assignment check. Also, this offered the opportunity not to diagnose assignments to final parameters, which I felt were underlined enough by the 'assignment to final' error. On the latter point, a case of interest would be an assignment to a final parameter within unreachable code. The current implementation raises an 'assignment to final' error, hence I believe there is not much value into raising a supplementary 'parameter assignment' warning.
(In reply to comment #9) > Review: > > Batch command-line argument should be aligned with existing ones > "parameter-assignment" --> "paramAssign" Next patch will implement the name 'paramAssign' for the batch command-line argument. Other associated constants (numeric and non numeric) names won't be changed.
Created attachment 30477 [details] New patch, integrated Eclipse format, for second review
Until JDT UI surfaces this option, it can be exerciced by assigning a value to org.eclipse.jdt.core.compiler.problem.parameterAssignment within the JDT Core preferences. For example, add the following line: org.eclipse.jdt.core.compiler.problem.parameterAssignment=warning into: .metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.jdt.core.prefs under Linux (relatively to the workspace root). The same with \ instead of / under Windows.
Added tests: AssignmentTest#test040-43 BatchCompilerTest#test037
Entered bug 122635 to ask for the addition of the new option to compiler preferences in the UI.
Created attachment 32461 [details] Committed changes (or very close to it)
Sorry, I'm a bit late. But do we really need this new option? What's wrong with assigning a parameter? And why is setting a parameter to final too much work? (you can vote for bug 108558 if you want us to automatically make parameters final). In the end every new compiler warning adds complexity, in the compiler and in the UI.
Philippe asks that the warning token be suppressed (too specific - would prefer some 'code-style' generic approach).
Clarification: - we keep the batch compiler warn option (aka -warn:+paramAssign); - we remove the warning token (aka @SuppressWarning(paramAssign)).
Created attachment 32684 [details] HEAD - suppressed warning token and improved build notes
Suppressed warning token, improved build notes, added JavaCore constant declaration and improved JavaCore#getDefaultOptions comment.
Verified for 3.2 M5 using build I20060214-0010
