Fixes creating constraint string from type parameter symbol #44
Conversation
| if (typeParameter.HasConstructorConstraint) | ||
| { | ||
| constraintString.Append(", new()"); | ||
| list.Add("new()"); | ||
| } | ||
| if (typeParameter.HasNotNullConstraint) | ||
| { | ||
| constraintString.Append(", notnull"); | ||
| list.Add("notnull"); | ||
| } | ||
| if (typeParameter.HasReferenceTypeConstraint) | ||
| { | ||
| constraintString.Append(", class"); | ||
| list.Add("class"); | ||
| } | ||
| if (typeParameter.HasUnmanagedTypeConstraint) | ||
| { | ||
| constraintString.Append(", unmanaged"); | ||
| list.Add("unmanaged"); | ||
| } | ||
| if (typeParameter.HasValueTypeConstraint) | ||
| { | ||
| constraintString.Append(", struct"); | ||
| list.Add("struct"); | ||
| } |
There was a problem hiding this comment.
The issue was that each block after the one above assumed that there was already a constraint and pre-pended a comma.
Instead of doing that, we collect the constraints on a list and then join them together.
There was a problem hiding this comment.
Makes sense. Good fix.
In the new baselines in this PR, I don't see any cases where constraints are joined with commas. Do we definitely have coverage of this?
There was a problem hiding this comment.
Also the original issue that this fixes reports the following error:
error CS0449: The 'class', 'struct', 'unmanaged', 'notnull', and 'default' constraints cannot be combined or duplicated, and must be specified first in the constraints list.
The code above doesn't seem to ensure that class/struct/etc would appear first in the list. Is it possible that this line (line 276) should be moved to the end of the list, or is it impossible for us to ensure correct ordering?
There was a problem hiding this comment.
Yeah, I've changed this after the fact, I've been adding more tests
| @@ -270,36 +270,35 @@ private void CreateTypeParameterProperty(TagHelperDescriptorBuilder builder, ITy | |||
| // things like constructor constraints and not null constraints in the | |||
| // type parameter so we create a single string representation of all the constraints | |||
| // here. | |||
| var constraintString = new StringBuilder(); | |||
| if (typeParameter.ConstraintTypes.Any()) | |||
| var list = new List<string>(); | |||
There was a problem hiding this comment.
We could remove an allocation by lazily initializing this, assuming this code path is always called and not only if the parameter has some constraint. An alternative would to be add a AppendToStringBuilder that conditionally adds the commas to avoid the interim string allocation from string.Join that happens later on in this method.
There was a problem hiding this comment.
Not going to remove the allocation unless we have prove that it matters in a benchmark. This code used to work and we broke it for the sake of removing the allocation without a clear benefit. Let's keep the code simple and understandable (and hopefully bug free)
javiercn
force-pushed
the
javiercn/fix-constraint-parameter-symbol
branch
from
88b2828 to
d1a8a9b
Compare
| where TItem1 : Image | ||
| where TItem2 : ITag | ||
| where TItem3 : Image, new() | ||
| where TItem1 : global::Image |
There was a problem hiding this comment.
This doesn't look correct since it's not a FQN.
There was a problem hiding this comment.
var classes = @"
public class Image
{
public string url { get; set; }
public int id { get; set; }
public Image()
{
url = ""https://example.com/default.png"";
id = 1;
}
}
public interface ITag
{
string description { get; set; }
}
public class Tag : ITag
{
public string description { get; set; }
}
";
AdditionalSyntaxTrees.Add(Parse(classes));
There was a problem hiding this comment.
Keep in mind that this data comes from the resolved symbol, so it can't produce the wrong output (otherwise it would be a bug in Roslyn)
|
🆙📅 this now includes an additional fix that we missed (since the fix is contained on the same method and we get it for free) We were not properly generating the type names in the where clause, and this manifested on type and interface constraints that were not being qualified appropriately as well as on self referencing constraints that weren't also being properly generated either. We switched to use the logic from roslyn to generate the display name for fully qualified type name of the constraint. In addition to that, we reordered the constraints to make sure that they appear in the right order. This fixes first the issue with the generated constraint when there was no base type, as well as the issue with specifying a base type or interface constraints |
javiercn
force-pushed
the
javiercn/fix-constraint-parameter-symbol
branch
from
d1a8a9b to
64c9f3d
Compare
Fixes dotnet/aspnetcore#38479 dotnet/aspnetcore#25588