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

Suggestion: Duck-typing ValidateProperty attribute #7835

Closed
Jaykul opened this issue Sep 21, 2018 · 20 comments
Closed

Suggestion: Duck-typing ValidateProperty attribute #7835

Jaykul opened this issue Sep 21, 2018 · 20 comments
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@Jaykul
Copy link
Contributor

Jaykul commented Sep 21, 2018

I'd like to propose adding a ValidateProperties attribute which would make it easier to validate that an incoming object had specific properties.

Hypothetically, use of it might look something like this:

[Parameter()]
[ValidateProperty(@{ SourceFile = [string]}, @{SourceLineNumber = [int]})]
[PSObject]$InputObject

The idea being that this would ensure that the InputObject has a SourceFile property of type string and a SourceLineNumber property of type int ...

Or you might even just do:

[Parameter()]
[ValidateProperty("SourceFile", "SourceLineNumber")]
[PSObject]$InputObject

Which would just validate the properties exist, and not their types.

We might even extend it with a NotNullOrEmpty check ...

Obviously I can add this in an external module, and I can do this validation with a ValidateScript ... but it seems like something that's generally useful (along the lines of the [PSTypeName()] attribute, but with the focus being on the shape of the object, rather than the name.

Thoughts?

@vexx32
Copy link
Collaborator

vexx32 commented Sep 21, 2018

As an extension of the available [PSTypeName()] this would be hugely useful. Love it.

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 23, 2018

Perhaps you'd want to apply other validate attributes to properties.

[Parameter()]
[Property(
    [ValidateRange(0, 10)]
    $Count,  
    [Mandatory = $true, ValidateNotNull()]
    $Name
)]
[PSObject]$InputObject

@iSazonov iSazonov added WG-Engine core PowerShell engine, interpreter, and runtime Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif labels Sep 23, 2018
@vexx32
Copy link
Collaborator

vexx32 commented Sep 23, 2018

Now that is a fun idea! Love it! That might have to come enclosed in a script block to avoid needing to create a dozen special cases for the parser, though!

@powercode
Copy link
Collaborator

It will be hard to implement efficiently. Member lookup on PSObject is currently very expensive, including quite a lot of allocations. Could probably be solved if we add new operations of PSObject, that doesn't retreive matching properties, but just confirms their existence.

For CLR members, we could cache results based on type, but for PSObjects it's more difficult.

I like the idea, though. People concerned about performance have other options.

@iSazonov
Copy link
Collaborator

It will be hard to implement efficiently.

We could consider this as proactive vs reactive. You already implemented efficient proactive approch to create a set of similar objects. If users can not use this approach they need reactive approach which is similar to the proposed here. PowerShell engine should be smart enough to turn off the second when using the first.

@lzybkr
Copy link
Member

lzybkr commented Sep 24, 2018

I'd suggest using class syntax, much like you'd specify a hypothetical interface that inputs could implement.

I use this basic idea in my prototype formatting engine, you can see examples here.

Applying the idea here, it might look like:

class SourceInfoProxy
{
    [string]$SourceFile
    [int]$SourceLineNumber
}

function foo
{
    param(
        [Parameter()]
        [ValidateDuckType([SourceInfoProxy])]
        [PSObject]$InputObject
    )
}

# or maybe the following - which could convert after validation,
# so maybe it's an ArgumentTransformation attribute instead.
function foo
{
    param(
        [Parameter()]
        [ValidateDuckType()]
        [SourceInfoProxy]$InputObject
    )
}

You could go one

@Jaykul
Copy link
Contributor Author

Jaykul commented Sep 24, 2018

I like that idea, @lzybkr ... but now I have an even better idea than an attribute ...

In the second example, as you say, it's an ArgumentTransformation attribute. It's basically a parameter-binding version of injecting a Select-Object and a cast.

In the past I've written reshaping filters like this:

filter ConvertTo-SourceInfoProxy {
[CmdletBinding()]
param(
    [Parameter(ValueFromPipelineByPropertyName)]
    [Alias("Line","LineNumber")]
    [int]$SourceLineNumber,

    [Parameter(ValueFromPipelineByPropertyName)]
    [Alias("Path","PSPath", "FullName")]
    [string]$SourceFile
)
[SourceInfoProxy]$PSBoundParameters
}

What if we could get all of that functionality (including aliases) into the parameter binding itself?

The [Alias] property can be applied to properties in classes (although it doesn't currently seem to do anything). I could, therefore trivially generate the filter above, if we had written the class this way:

class SourceInfoProxy {
    [Alias("Line","LineNumber")]
    [string]$SourceLineNumber
    [Alias("Path","PSPath", "FullName")]
    [int]$SourceFile
}

So what we would need is just the ability to either specify a [Coerce()] ArgumentTransformation attribute that just coerces anything that can be coerced. Or maybe instead, we could just generate cast operators (in powerShell, we implement this as a constructor, right?) on classes (for PSObject), so that the cast would be implicitly available?

Which brings me to ....

What if we had GO style interfaces, instead?

In Go, there's no need to say that you implement an interface, instead, if a type defines all the methods of an interface, it implicitly implements that interface.

Can we do that in PowerShell?

What if I could define SourceInfoProxy as an Interface and then just use it as a parameter or variable type and PowerShell automatically validates the properties (and methods?) and even generates property aliases if necessary -- all while leaving the original object intact?

This way we wouldn't be throwing away data by coercing the object ...

@vexx32
Copy link
Collaborator

vexx32 commented Sep 24, 2018

So we'd have a [ValidateInterface([type])] sort of attribute? I'm down.

@Jaykul
Copy link
Contributor Author

Jaykul commented Sep 24, 2018

Well, if we had GO style interfaces, it would just be a parameter of type [ISourceInfoProxy] -- no need for an attribute. Otherwise, I think ... [Coerce([SourceInfoProxy])][SourceInfoProxy]

@vexx32
Copy link
Collaborator

vexx32 commented Sep 24, 2018

Mmm. That would mean that casting to an interface would have to behave very differently in PS to how it does in C#.

Not really sure that's a good idea...

@Jaykul
Copy link
Contributor Author

Jaykul commented Sep 24, 2018

You're funny @vexx32 -- casting in PowerShell already behaves very differently than how it does in C# and casting to interfaces especially so. PowerShell ignores explicitly defined cast operators in favor of constructors and type coercion already -- and that's not even counting what happens when you cast hashtables...

@powercode
Copy link
Collaborator

@Jaykul Type conversion is very different from C#, but not much of that applies when the target type is an interface, does it?
If I remember correctly, it is only IsAssignableFrom and registered converters that applies. All the other Create/Parse/Ctor stuff doesn't apply to interfaces.

@lzybkr
Copy link
Member

lzybkr commented Sep 24, 2018

To be more specific, PowerShell usually uses the runtime type (obj.GetType()) of an object, which is almost never an interface (obj.GetType() can return an interface for .Net remoting, but otherwise it doesn't.)

So a "conversion" to an interface is very different than in C#. PowerShell just confirms the object implements the interface, but subsequent uses of the object can access members non-interface members without additional conversions.

@Jaykul
Copy link
Contributor Author

Jaykul commented Sep 25, 2018

Right -- and my point is ... what if we just changed our definition of "implements the interface" to mean "it looks like a duck, and it quacks like a duck"

@vexx32
Copy link
Collaborator

vexx32 commented Sep 25, 2018

How does c# determine things like this? The Deconstruct method seems to be a salient example - implementing it allows you to treat any object or struct like a deconstuctible tuple-of-sorts which doesn't have to implement any particular interface.

@KirkMunro
Copy link
Contributor

KirkMunro commented May 17, 2019

@vexx32 Since you've started the PR process, I wanted to share a few thoughts on this:

  1. I think this may make for a good RFC discussion. There are many possibilities proposed here, so it would be useful to open the discussion further via an RFC, present all options, and see how the community feels before running forward with the design. Unless of course there is consensus on this issue, but with the collection of proposals here there doesn't seem to be consensus at this time.

  2. As for syntax, since this seems to be intended for use when working with an object that is not strongly typed, I'd honestly prefer this:

    [PSObject(PropertyName1,PropertyName2)]
    or this:
    [PSObject([string]PropertyName1, [int]PropertyName2)]

    That syntax feels easier to digest by keeping it simple, it's more inline with the design of the PSTypeName attribute, and saves having to add another attribute when you want a PSObject with specific properties/types on it.

  3. I don't think this should check anything more than type (i.e. no validaterange, etc.). If users want more validators, they can have multiple parameters with ValueFromPipelineByPropertyName, and validate whatever they want. Otherwise this gets quite confusing.

@vexx32
Copy link
Collaborator

vexx32 commented May 17, 2019

  1. Yes, possibly. I'm more than willing to open an RFC in tandem, but I figured we may as well have some code backing it up to begin with.

  2. I like the idea of this, but I don't see how that could be supported syntax wise without making breaking changes to the AST & visitors for the attributes in general.

  3. Definitely agreed. If users want that, I believe it can be done with classes anyway.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

1 similar comment
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants