[Refactoring] Names and Name cleanup (use static access & getter)

[Shadow-Devil]

Purpose

• Made all methods of the Names class static, since they access no instance fields.
• Remove all references to local Names variables and only use static access
• Encapsulate Name.value field (make it private and only use getValue()).

Check List

• Read the Contributing Guide

[codecov]

Codecov Report

Attention: Patch coverage is 82.80961% with 186 lines in your changes missing coverage. Please review.

Project coverage is 77.49%. Comparing base (b45e1e1) to head (ff36b51).
Report is 25 commits behind head on master.

Files with missing lines	Patch %	Lines
...alang/compiler/semantics/analyzer/SymbolEnter.java	69.41%	22 Missing and 4 partials ⚠️
...g/wso2/ballerinalang/compiler/desugar/Desugar.java	80.55%	8 Missing and 6 partials ⚠️
...va/org/wso2/ballerinalang/compiler/bir/BIRGen.java	91.35%	6 Missing and 1 partial ⚠️
...alang/compiler/desugar/DeclarativeAuthDesugar.java	14.28%	5 Missing and 1 partial ⚠️
...ng/compiler/semantics/analyzer/SymbolResolver.java	68.42%	3 Missing and 3 partials ⚠️
...lerinalang/compiler/bir/codegen/JvmPackageGen.java	73.68%	0 Missing and 5 partials ⚠️
...rinalang/compiler/packaging/repo/PathBalaRepo.java	0.00%	4 Missing ⚠️
...lerinalang/compiler/packaging/repo/RemoteRepo.java	0.00%	4 Missing ⚠️
...mpiler/semantics/analyzer/ConstantTypeChecker.java	75.00%	4 Missing ⚠️
...llerinalang/compiler/semantics/analyzer/Types.java	71.42%	3 Missing and 1 partial ⚠️
... and 65 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #43265      +/-   ##
============================================
- Coverage     77.51%   77.49%   -0.02%     
+ Complexity    58585    58578       -7     
============================================
  Files          3438     3438              
  Lines        219219   219237      +18     
  Branches      28921    28925       +4     
============================================
- Hits         169921   169896      -25     
- Misses        39884    39925      +41     
- Partials       9414     9416       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

[heshanpadmasiri]

@Shadow-Devil first of all thank you very much for taking time to do this refactoring and I generally agree with almost all the changes, with one minor caveat. You may have seen in many places we have direct access to fields instead of going via encapsulated methods. Rational for this is as fallows

1. If we do a direct field access we get a getField instruction instead of invokeVirtual instruction which is faster from the get go
2. C2 can optimise invokeVirtual in the hot sections to be just as fast as getField assuming class is final (otherwise it has to do a class word check even if we don't actually extend the class since we could be loading classes dynamically from the JVMs perspective)
3. But in most cases (with exception being the parts used by language server) our compiler don't run long enough to be fully "jitted" (so back to 1)

Given with your changes Name.value is anyway final (and its a string which is immutable) I think the only benefit we can get by using the encapsulated method is,

1. We can add some validation or other logic to the accessor in the future
2. We can use the interface (note invokeInterface as far as I know don't get same treatment as invokeVirutal when there is a single class implementing it, so it would always has the class word check even after "jitting")

Therefore I think it is better to cross that bridge when we need to do 1 since I don't see in many places we need to do 2. WDYT?

[Shadow-Devil]

@heshanpadmasiri oh wow, thank you for that deep analysis! Do you know what would happen, if we made Name a record? then one would have to use the accessors and (i hope) the invokeVirtial could be optimised away. Downside is, that the methodname would change from getValue() to value() and many libraries which use this field would need to be updated as well. WDYT?

//Edit: I just tried it and record field access also generate an invokeVirtual call instead of a getField which is sad...

[Shadow-Devil]

As I understand the Name class is pretty much only a wrapper of a String.
This would be nicely modeled by Kotlins inline value classes which will optimize the wrapper class while compiling but still offering typesafety.
In Java there is also work in this direction with JEP 401 but this is currently still a draft.

[Shadow-Devil]

@heshanpadmasiri If you think we should not do this change, close the PR please so I know that this will not be included. I do understand your points but still think these micro optimisations might not be enough to go against the Java Standard of having getters instead of directly accessing the property. Maybe also others could share their opinion on this topic :)