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

Detect wrong argument constraint type at runtime #1237

Closed
thomaslevesque opened this issue Oct 16, 2017 · 12 comments
Closed

Detect wrong argument constraint type at runtime #1237

thomaslevesque opened this issue Oct 16, 2017 · 12 comments
Assignees
Milestone

Comments

@thomaslevesque
Copy link
Member

thomaslevesque commented Oct 16, 2017

Related to #1177, #1230, #1163

We already have the FakeItEasy0005 diagnostic to detect this, but we can also detect it at runtime, and we should throw in this case.

A possible approach would be to examine all the constraints collected by IArgumentConstraintTrapper.TrapConstraints and check if their type is compatible with the parameter type.

@blairconrad
Copy link
Member

I like the idea of simply rejecting all expressions that are conversions in CheckArgumentExpressionIsValid.
Sadly, we already allow the user to pass a T constraint in the place of a Nullable<T> parameter.

@thomaslevesque may be right: we might need to investigate the types that we get from the constraint trap.
Of course, we could just do this when the expression is a conversion. I'll play a bit.

@thomaslevesque
Copy link
Member Author

From #1230:

True. I have started working on #1237

Oh. So have I. It was unowned, so I self-assigned and started working on it last night.

As you like @blairconrad. I'm mostly done, but I haven't written any tests yet, and I won't have time to work on it for a few days. If you want to look at what I have, here it is: thomaslevesque@41e6011

I added a new Type property to IArgumentConstraint in order to be able to compare against the actual parameter type.

@blairconrad
Copy link
Member

Oh, man. My approach was going to be totally different. Which probably means I'm forgetting lots of stuff! I don't mind just noodling along as a hobby even if/when we take your fix.

I'm going to be away tomorrow and then Friday through Sunday as well, so who knows when I'll be done…

@thomaslevesque
Copy link
Member Author

My approach was going to be totally different

I look forward to seeing what you had in mind ;)

@blairconrad
Copy link
Member

blairconrad commented Oct 19, 2017

Well, here it is. Very rough. Tests at least need review and the production code needs refactor.
blairconrad@99b6a80

@thomaslevesque
Copy link
Member Author

It's an interesting approach. I like the fact that it doesn't require as much changes as mine. On the other hand, it doesn't detect wrong types in hidden nested constraints, e.g. constraints created by user-defined methods.

@blairconrad
Copy link
Member

Can you gimme an example of such a thing?

@thomaslevesque
Copy link
Member Author

Something like this:

public static class Extensions
{
    public static int IsEven(this IArgumentConstraintManager<int> manager)
    {
        return manager.Matches(x => x % 2 == 0);
    }
}

Or even:

private static int AnEvenNumber()
{
    return A<int>.That.Matches(x => x % 2 == 0);
}

@blairconrad
Copy link
Member

Ugh. You're right, of course. I need to look at your solution more closely to see how it works. I have to relearn argument constraint trapping every single time I look at it.

@thomaslevesque
Copy link
Member Author

I need to look at your solution more closely to see how it works. I have to relearn argument constraint trapping every single time I look at it.

You don't really need to understand it... I didn't touch how constraints are trapped, I'm just examining the results.

@blairconrad
Copy link
Member

I found my refresher to be useful anyhow. Your approach has more promise. Abandoning mine.

@blairconrad
Copy link
Member

This change has been released as part of FakeItEasy 4.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants