Don't fail for Razor: SymbolReference locations for @typeparam are misplaced #8424
Conversation
martin-strecker-sonarsource
changed the title
| } | ||
| } else if (declarationRange.get().overlap(referenceRange.get())) { | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("The declaration token at {} overlaps with the referencing token {} in file {}", declarationRange.get(), referenceRange.get(), file.filename()); |
There was a problem hiding this comment.
The actual fix.
There was a problem hiding this comment.
what is the actual fix here? It's hard for me to follow what the difference is from the diff
There was a problem hiding this comment.
It's the condition from the if statement: declarationRange.get().overlap(referenceRange.get()). If we add the range directly, the API we use (symbol.newReference) will throw.
So this check ensures that we log a warning (in debug) and continue the import ignoring the second range that is overlaping.
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| public class RazorImporterTestBase { |
There was a problem hiding this comment.
New base class for all tests that interact with "src/test/resources/RazorProtobufImporter" files.
| Tuple.tuple(CoreMetrics.CLASSES, 1), | ||
| Tuple.tuple(CoreMetrics.STATEMENTS, 15), | ||
| Tuple.tuple(CoreMetrics.FUNCTIONS, 4), | ||
| Tuple.tuple(CoreMetrics.COMPLEXITY, 5), | ||
| Tuple.tuple(CoreMetrics.FUNCTIONS, 3), | ||
| Tuple.tuple(CoreMetrics.COMMENT_LINES, 0), | ||
| Tuple.tuple(CoreMetrics.COGNITIVE_COMPLEXITY, 1), | ||
| Tuple.tuple(CoreMetrics.NCLOC, 14)); | ||
| Tuple.tuple(CoreMetrics.CLASSES, 0), | ||
| Tuple.tuple(CoreMetrics.NCLOC, 13), | ||
| Tuple.tuple(CoreMetrics.STATEMENTS, 6)); |
There was a problem hiding this comment.
Some values changed, likely because the pd files were created new, and the old ones didn't fit "Cases.razor" or because the old ones were created with .Net 7 SDK and the new ones with .Net 8 SDK.
| var referenceRange = toTextRange(file, refTextRange); | ||
| if (referenceRange.isEmpty()) { | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("The reported token was out of the range. File {}, Range {}", file.filename(), refTextRange); |
There was a problem hiding this comment.
We lost coverage for that line because in the new pb, there is not "out of range" anymore (either fixed in .Net 8 SDK or fixed somewhere else).
There was a problem hiding this comment.
They fixed the tab problem and we used tabs to reproduce that out-of-range issue.
github-actions
bot
assigned sebastien-marichal and unassigned martin-strecker-sonarsource
martin-strecker-sonarsource
added
Type: Bug
|
SonarCloud Quality Gate failed.
|
|
Kudos, SonarCloud Quality Gate passed!
|
martin-strecker-sonarsource
requested a review
from costin-zaharia-sonarsource
github-actions
bot
assigned costin-zaharia-sonarsource and unassigned martin-strecker-sonarsource
martin-strecker-sonarsource
deleted the
Martin/SymbolReferenceProtobuf
branch
| Roslyn version: 4.8.0.0 Language version: CSharp12! Concurrent execution: enabled��File 'Microsoft.NET.Sdk.Razor.SourceGenerators\Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator\OverlapSymbolReferences_razor.g.cs' was recognized as razor generated��File 'Microsoft.NET.Sdk.Razor.SourceGenerators\Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator\Cases_razor.g.cs' was recognized as razor generatedvrFile 'C:\Users\martin.strecker\Desktop\WebAss\obj\Debug\net8.0\WebAss.AssemblyInfo.cs' was recognized as generated��File 'C:\Users\martin.strecker\Desktop\WebAss\obj\Debug\net8.0\.NETCoreApp,Version=v8.0.AssemblyAttributes.cs' was recognized as generated |
There was a problem hiding this comment.
double check I understand well: did you recreate these PB files with .NET 8 @martin-strecker-sonarsource ?
There was a problem hiding this comment.
Yes. I also added the csproj as part of the PR, so it is more easy to recreate the pb files (before, the csproj was missing):
martin-strecker-sonarsource
assigned martin-strecker-sonarsource and unassigned costin-zaharia-sonarsource
|
Peach validation: Other references work as well. See e.g. the |
It's just a type and I do not know where it is defined. It is highly likely defined either in the project or in the framework. I just wanted to show that symbol references work for the properties in that "normal" C# file:
|
| @@ -0,0 +1,6 @@ | |||
| namespace WebAss; | |||
There was a problem hiding this comment.
The first chars look strange. Is the encoding set correctly on the file?
| .setMetadata(new FileMetadata(mock(AnalysisWarnings.class)).readMetadata(new FileReader(TEST_FILE))) | ||
| .build(); | ||
| sensorContext.fileSystem().add(inputFile); | ||
| public void test_symbol_refs_get_imported_cases() { | ||
|
|
There was a problem hiding this comment.
pedantic: there is an empty line at the beginning of the tests that is not needed
| public void test_symbol_refs_get_imported_overlapSymbolReferences() { | ||
|
|
||
| var inputFile = OverlapSymbolReferencesInputFile; | ||
| var sut = new SymbolRefsImporter(sensorContext, s -> Paths.get(s).getFileName().toString()); |
There was a problem hiding this comment.
var sut = new SymbolRefsImporter(sensorContext, RazorImporterTestBase::fileName);
| var sut = new SymbolRefsImporter(sensorContext, RazorImporterTestBase::fileName); | ||
| sut.accept(protobuf.toPath()); | ||
| sut.save(); |
There was a problem hiding this comment.
Creating the sut is duplicated (but slightly different), we should extract a function to create it.
See #8417
The PR turns the hard failure into a log message.