search

Unity-Technologies / ProjectAuditor

Project Auditor is an experimental static analysis tool for Unity Projects.
Other
806 stars 66 forks source link

Obsolete API detection #88

Closed mtrive closed 2 years ago

mtrive commented 2 years ago

This PR introduces support for detecting calls to methods marked with ObsoleteAttribute.

Note that all reported ObsoleteAttribute diagnostic issues will be reported as either Warnings or Errors. Depending on the attribute constructor arguments.

obsolete-method-v2

Note this was a feature request by @jed-unity3d

mtrive commented 2 years ago

These are very pertinent questions. I am glad you asked them.

I have questions.

  1. Is there a reason why Obsolete attribute detector isn’t implemented as an IInstructionAnalyzer like the rest of the custom code analyzers are? Why is is hard-coded in CodeModule.cs and have hard-coded analysis logic in there as well?

Initially this feature was reporting all methods tagged with Obsolete, regardless of whether they are called (which arguably is not very useful). IInstructionAnalyzerdon't serve that use-case so it had to be implemented at a higher level. Now that we actually report the calls to those methods, using an IInstructionAnalyzer makes sense.

  1. What are the yellow warning triangle next to calls to Obsolete methods? I see you’ve added a Severity column to the view/report but I don’t know why Obsolete calls get an icon when other things don’t.

This is a new type of diagnostic, perhaps code-quality related. So far all reported diagnostics typically affect performance or memory and it's up to the user to determine whether they are actual issues and worth fixing. I think it's fair to say the Obsolete attribute should always be considered a Warning, unless specified otherwise and in that case it's an Error (see Obsolete constructor arguments). So, I think the Severity column makes sense here. Should we take advantage of that for all other diagnostics? I guess that's the open question. Alternatively, should we move Obsolete issues to a different view? (perhaps not now but in the future if we support more diagnostics of this kind)

  1. I’m not a fan of the way the Issue strings appear. By way of example in your screenshot, I think the first method’s issue should just be “Call to ‘OnPopulateMesh’ obsolete method”. Or possibly just “Graphic.OnPopulateMesh”. Then the Details window should say “This method is marked as obsolete” (as it is now) and the recommendation should be the string from the attribute - in this case “Use OnPopulateMesh(VertexHelper vh) instead.”. Would this be possible? Are there any reasons that this might not be a good idea?

I agree. I am not completely happy about this. It makes more sense to use the side panels to display the Obsolete attribute message. Regarding the main description I think we should use “Call to ‘OnPopulateMesh’ obsolete method”. I know that, now, most API calls are reported by simply using the "type.method" as description. This should probably be slightly more descriptive, like "type.method usage" / "call to type.method". This gives you more options when searching and the report is clearer when exported or when using the "flat view" mode (as issues are not organized by descriptor).

mtrive commented 2 years ago

@unitystevem here is how it looks now.

obsolete-method-v3

mtrive commented 2 years ago

Turns out we don't need to distinguish between warnings and errors because only non-fatal (warnings) calls to obsolete methods are reported in the Code View (we can only analyze assemblies that compiled successfully). So we don't need the Severity column at all. In addition, both warnings and errors are reported by the complier and captured in the Compiler Messages view.

Considering these are already reported elsewhere I don't think we gain much by adding non-fatal calls to Obsolete methods to the Code View. I am going to close this PR for the time being. If we come up with a strong argument for actually landing this feature we will reopen it.