Test case for #1180 and improve NullableType.ToString

[delta-emil]

Test case for #1180

This allows comparing vague SqlTypes (without Length/Precision/Scale defined), which is what you get for constants, to always be considered equal to their more particular counterparts. Instead of only if they are on the left hand side of the comparison.

Also added an override of ToString for NullableType, so that type-related error messages, like the one in ConditionalProjection give useful information about types differing only by their SqlType.

[bahusoid]

to always be considered equal to their more particular counterparts. Instead of only if they are on the left hand side of the comparison.

But why they should be considered equal? Should not they be always considered not equal on both sides of comparisons instead?

And for #1180 I think we should extend ctor to allow to provide return type explicitly.

[delta-emil]

But why they should be considered equal? Should not they be always considered not equal on both sides of comparisons instead?

That is an interesting question. When I saw the comparison in SqlTypes's Equals, I assumed the intent was to allow a loose type to be equal to any of its stricter versions, but someone just forgot to make it symmetrical.

In the case of the ConstantProjection inside a ConditionalProjection, forcing the user to explicitly give provide the IType of the ConstantProjection or the ConditionalProjection, seems a bit like an unnecessary annoyance, when the value inferred by ConstantProjection internally calling NHibernateUtil.GuessType is good enough for the job.

It would definitely force me to do a lot more work in one particular use case, where I have code dynamically put together a query, but I'll deal with it, if the community feels that's the way to go. Probably by making a custom variation of ConditionalProjection for that place.

Another option would be to have a separate "LooseEquals" for cases like the ConditionalProjection comparing the types of its two result projections? Or maybe have ConditionalProjection only be a bit loose with the type comparison when a ConstantProjection is involved?

[bahusoid]

forcing the user to explicitly give provide the IType of the ConstantProjection or the ConditionalProjection, seems a bit like an unnecessary annoyance,

I meant it as optional parameter. So user should provide it only in case of ambiguity. So I don't see it as much of annoyance.

Another option would be to have a separate "LooseEquals"

I would call it IsTypeCompatible or something like that. And IMO it requires additional thinking. To me it seems it's asymmetrical thing by nature - 10-length string is compatible with 1-length string for output parameter. But it's not true otherwise. And it's completely different story for input parameters. And if it's required only for this case I don't think it's worth to be implemented inside NHibernate.

[delta-emil]

Shall I update the change to resolve the Equals asymmetry in the direction of strict checks? That will at least make things act consistently when swapping spots, and provide a better error message, when types don't strictly match.

Deciding on where to go with ConditionalProjection's GetTypes can maybe be done separately by someone with better understanding of when and how the IType and SqlType instances are used. After all, if #2460 gets merged, ConditionalProjection's GetTypes won't even get called in the case I came across. So I won't even know what test case to write.

[bahusoid]

Shall I update the change to resolve the Equals asymmetry in the direction of strict checks?

Yes let's try to make it strict. I wonder if it breaks anything in our tests.

[bahusoid]

@hazzik Why approved? Have you read the discussion? In current state this PR makes types with defined Length (or Precision or.. ) be always equal to type without defined Length. That's IMO not the right behavior - 1 length string type should not be equal to string type without length defined.

[hazzik]

Code looks correct-ish, so I approved. Yes, I've read the discussion.

.

[hazzik]

@bahusoid can you finialize this?

[bahusoid]

can you finialize this?

I think this PR can be committed only as unit tests. As suggested fix is incorrect and proper asymmetrical fix won't fix #1180

[bahusoid]

Approve unit tests and better error message changes.

And it seems fix should implement "loosen" equality right inside ConditionalProjection (maybe just check that NullableType.ReturnedClass is the same?)