Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.razor: implement the token type metrics #7895

Open
costin-zaharia-sonarsource opened this issue Aug 25, 2023 · 4 comments
Open

.razor: implement the token type metrics #7895

costin-zaharia-sonarsource opened this issue Aug 25, 2023 · 4 comments
Assignees
Labels
Area: C# C# rules related issues. Type: Web Improve ASP.NET, Razor, Blazor or similar rules.

Comments

@costin-zaharia-sonarsource
Copy link
Member

costin-zaharia-sonarsource commented Aug 25, 2023

Description

See the "Token type" section from the specification document.
More details about the approach here.

Alternative

An alternative to this approach, which only focuses on mapping back tokens without altering the fundamental behavior of the Token Type Analyzer, is to use the RazorSyntaxTree parsing. That approach, that is more powerful but also more complex to implement, is captured by a dedicated issue.

Update

Moving back to "To do" waiting for the issue with location mapping for nested Razor expression to be fixed first, since wrong location mapping can produce wrong and overlapping tokens, and those break tokenization import on SonarQube/SonarCloud by the sensor.

Update 2024

Underlying PR closed for the reasons explained #7924 (comment).
For a new implementation, please consider the analysis and the scenarios described in the description of the first PR.

@antonioaversa
Copy link
Contributor

Handed over to @costin-zaharia-sonarsource. The underlying PR describes the current status and the next steps.

@antonioaversa
Copy link
Contributor

Took it over from @csaba-sagi-sonarsource.
After discussing with @csaba-sagi-sonarsource and @costin-zaharia-sonarsource, I am going to:

  • rebase onto the current master and fix conflicts
  • extend unit tests to include missing Razor features (e.g. nested components)
  • making sure everything works as expected and have a third round of review from @costin-zaharia-sonarsource
  • if time allows, check locally the performance delta (overall build time on mud-blazor) w.r.t. master (before master validation on peach)

@antonioaversa
Copy link
Contributor

@costin-zaharia-sonarsource
I am doing backlog cleanup, and I have a question about this issue and its related PR.

The issue has been hanging on the Kanban board, for a while now.
At the time we decided not to proceed with razor tokenization, due to the issues with locations we had at the beginning.
In the end, however, if I recall correctly, we went for our own custom mapping logic, so this task may make more sense now.
There are still problems with location mapping in nested components, as explained in the description of the PR.
As mentioned here, #8040 should be addressed first.

There is quite a lot of code, and the PR went also for a round of review.
However, the branch has become stale, and by now there are 10 conflicts with master.

How would you advise to proceed?

@costin-zaharia-sonarsource
Copy link
Member Author

Since there are still problems with the mapping and the Roslyn APIs don't help us much currently, I would prefer to pause this effort for now. In my opinion, we should wait for the compiler team to finish the current effort in improving the API and continue when the new registrations are available.

@costin-zaharia-sonarsource costin-zaharia-sonarsource added Type: Web Improve ASP.NET, Razor, Blazor or similar rules. and removed Type: New Feature labels Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Type: Web Improve ASP.NET, Razor, Blazor or similar rules.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants