Updates in performance highlightings.

[krasnotsvetov]

1. Add context "Move to start", "Move to awake" and "Move outside the loop" if current operation in loop. (All context actions is available only in per frame methods and methods that are reachable from them)
   a. Currently not solution-wide (only for opened file)
   b. This operations can break semantics, so they are just only context actions, not quickfixes
2. Add highlighting on gutter for each performance method
3. Add possibility to disable highlightings

[kskrygan]

please correct some minor issues related to threading and locks assertions

[kskrygan]

this is a public API.
threading?
locks?
Please use Shell.Instance.Locks.assertBlaBlaBla for all those purposes

[kskrygan]

seems it can be accessed from UI thread only. Please assert that.

[kskrygan]

please exclude this file from the merge

[kskrygan]

why is it so?

[krasnotsvetov]

Out-of-date 183's fixes. Fixed.

[kskrygan]

assertions

[TessenR]

It looks like QFs assume they are moving code between methods of the same type.
The analyzer on the other hand seems to check methods from the same file.

Please add tests for code like this:

class C : MonoBehaviour
{
  public void Update()
  {
    var t = new MyType();
    t.Test();
  }
}

class MyType : MonoBehaviour
{
  public void Test()
  {
    GetComponent("test");
  }
}

Is there a reason why a call Stack is not sufficient for the performance inspector?
It looks like there's a tremendous amout of work done for keeping CurrentElement and CurrentDeclaration consistent, reassignments and caching those properties just to check if the element is already processed, maintaining a call graph to propagate costly invocation mark backwards. It also seems like there are still possible pitfalls like visiting method declaration with incorrect values in .Current* properties or missing processed mark for local functions.

[TessenR]

The comment looks strange to me.
IInvocationExpression can be just a local function invocation in a correctly parsed code.
The reference expression can also resolve to almost anything e.g. fields, properties, local variables, parameters etc if they are delegates.
E.g.

Action a = () => { };
a();

Why mention parsing recover strategy here?

[krasnotsvetov]

Out-of-date comment, I have removed it :)

[TessenR]

It looks like the code above skips types nested in classlike declarations not inherited from MonoBehaviour which doesn't seem intended
e.g.

class C 
{
  class NotProcessed : MonoBehaviour { ... }
}

[krasnotsvetov]

It is not possible for Unity. MonoScript should be top-level class with same name as file name.

[TessenR]

It looks like partial methods will have one random declaration processed (probably not even containing the actual code)

[TessenR]

find all declarations in current file
.FirstOrDefault(...);

It looks like the comment is outdated.

I wonder why only the first declaration is processed though. It's possible to have multiple declarations of the same method with partial types. E.g.

partial class P
{
  partial void M();
}

partial class P
{
  partial void M() => Console.WriteLine();
}

Current code will process one random possibly empty declaration from the code above.

[TessenR]

Regarding GetDeclarationsIn(mySourceFile) call which returns declarations from other files - it looks like a copy-paste mistake in the source code for CSharpMethod. I'd recommend fixing the root problem instead of hacking it multiple times here.

[TessenR]

It looks like this code should process local functions when their invocations are found.
Is it intended to ignore costly calls within lambda / anonymous method / query expressions?
e.g.

void M()
{
  Action a = () => CostlyInvocation();
  a();
}

[TessenR]

I haven't seen any code that will mark local function invocations (or any closures, altough it seems that any othe forms of closures aren't processed) as visited as the MarkCurrentAsVisited method is only called within VisitMethodDeclaration. Therefore they'll probably be analyzed multiple times. While it won't cause stackoverflow exception due to the depth limit it can cause a huge performance hit especially in case of large recursive functions since they will be analyzed MaxDepth times for each call. Please check performance for code like this:

void M()
{
  Trigger(); // start inspecting the local function

  void Trigger()
  {
    Local(); // inspect the second local function
    Trigger(); // inspect Trigger and consequently Local again MaxDepth times
  }

  void Local()
  {
    // 1000+ lines of code here, inspect them all
    Local(); // inspect Local MaxDepth times recursively
  }
}

Will this analyze the Local source MaxDepth^2 times?

[TessenR]

.Copy()

[TessenR]

I wonder how this would/should work with code like this:

class MonoBehaviour
{
  public virtual extern void Start();
}

abstract class C : MonoBehaviour
{
  public abstract override void Start(); // move something to this method
}

[TessenR]

var localVariableDeclaration =
        LocalVariableDeclarationNavigator.GetByInitial(
          ExpressionInitializerNavigator.GetByValue(expression.GetContainingParenthesizedExpression()))

[TessenR]

.GetPresentableName() returns a string which should represent a human-readable type name in hints/warnings. It might not be a valid identifier e.g. it could return int[] or MyType<T>.
Does naming service handle such base names? Please, add tests for such scenarios.

[TessenR]

It looks like this method tries to emulate the following:

INamesCollection namesCollection;
namesCollection.Add(GetNameRootFromStringArgument(), EntryOptions { Emphasis = Good ... });
namesCollection.Add(expression, EntryOptions { ... }); // this will add roots based on the expression's type and text

INamesCollection already has the capability to suggest names based on the expression's type.
It also can suggest names based on the expression's code. It's ignored here altough I'm not sure if it's intended.