Pull Request Test Coverage Report for Build 3731314363

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.9%) to 96.898%

Totals
Change from base Build 3730045467:	0.9%
Covered Lines:	12478
Relevant Lines:	12725

💛 - Coveralls

From my comment in #2061 (comment)

E.g. in #2056 you're excluding decimal for the check if a Type is to be considered a struct.
I don't immediately know why that is, but it looks like a concern that could influence whether an API should be added and hence should be discussed in the issue instead of a PR.

In IL there is no direct concept the C# keyword struct but only value types which also includes structs.
https://learn.microsoft.com/en-us/dotnet/standard/base-types/common-type-system?redirectedfrom=MSDN#structures

public struct Foo{}

.class public sequential ansi sealed beforefieldinit Foo
    extends [System.Runtime]System.ValueType
{
    .pack 0
    .size 1

} // end of class Foo

SharpLab

So like IsCSharpAbstract and IsCSharpSealed we cannot exactly detect a 1:1 mapping between C# code written and the IL we have available.
But I think this case is more complicated, and I'm not convinced we should add this API.

Some points of the implementation

• Primitive types are not considered structs despite e.g. float being an alias for the struct System.Single
• System.Decimal is specially treated as a non-struct - I can only assume to behave like the other primitive types?
• The newly added System.Half does not have special treatment. I assume because System.Half was added in .NET 5 after https://stackoverflow.com/a/62727198 was written?
  • This is a simple case, but then how about any other custom numeric type? E.g. System.Numerics
  • SharpLab

To summarize:
There are several pitfalls which makes me think this API does not pull its own weight.
Not to sound (too) grumpy, but is why I like to discuss APIs in issues before rushing to implementation.

I see, there is really a problem with the half and complex types.
I totally agree and will close the PR

On the other hand, a lot of developers may not even be aware of all those special cases and just want to know which of their types in their code base are struct. They will never expect to find primitive types, decimal or Half in the results, so they will never run into those specific edge cases. So I still see value in having this method, provided that we add clear documentation on these edge cases.

On the other hand, a lot of developers may not even be aware of all those special cases and just want to know which of their types in their code base are struct. They will never expect to find primitive types, decimal or Half in the results, so they will never run into those specific edge cases. So I still see value in having this method, provided that we add clear documentation on these edge cases.

You are right, i also didn't know about these special types that C# offer, but to be honest i'm also new in C# .NET technologies and coming from java.

They will never expect to find primitive types, decimal or Half in the results

As we don't know all future or custom numeric types, we cannot filter them out.

To avoid any ambiguities we could have ThatAreValueTypes.

To avoid any the ambiguities we could have ThatAreValueTypes.

Yes, let's do that. @94sedighi what do you think?

To avoid any the ambiguities we could have ThatAreValueTypes.

Yes, let's do that. @94sedighi what do you think?

@dennisdoomen It will cover a wider range of types, but we can do it, and there are no edge cases, as far as I know the C#.