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

Add-Type now quietly ignores attempts to change a previously added .NET type's definition in the same session #9575

Open
mklement0 opened this issue May 10, 2019 · 16 comments

Comments

Projects
None yet
4 participants
@mklement0
Copy link
Contributor

commented May 10, 2019

This is a regression from Windows PowerShell, where the - useful - behavior is as follows:

  • If you repeatedly call Add-Type -TypeDefinition/-MemberDefinition for the same target type, only the first invocation adds the type to the session and subsequent calls are benign - and fast - no-ops, if the source code is unchanged.

  • If you modify the source code, you get an error along the lines of:
    Add-Type : Cannot add type. The type name '<name>' already exists.

This behavior is sensible, as it alerts you to the fact that trying to change the type in-session isn't possible.

By contrast, PowerShell Core now quietly ignores attempts to change the type, which means that you may not notice that your changes didn't take effect.

Steps to reproduce

Run the following Pester test

{
  Add-Type -TypeDefinition @'
  public class Foo {}
'@
  
  # Try to redefine the type with different source code.
  Add-Type -TypeDefinition @'
  public class Foo { public int Bar {get {return 42;} } }
'@
  
} | Should -Throw

Expected behavior

The test should succeed, because an error should be emitted.

Actual behavior

The test fails, because no error is emitted, because the second Add-Type is effectively quietly ignored [a new assembly is actually being created - see
@daxian-dbw's comment below]

Environment data

PowerShell Core v6.2.0
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

Add-Type was fully re-implemented on Roslyn (CodeDom was used in Windows PowerShell).
And now we actively use caching - if we can find a type we use them and don't compile again. If I remember right this was accepted as preferred behavior.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@iSazonov:

Windows PowerShell is capable of detecting if the source code changed and only if it did does it complain - helpfully so; otherwise, the call is a quiet - and fast - no-op.

Are you saying the same can't be done in PowerShell Core?

I don't think anyone prefers the behavior of having their attempt to define a type quietly ignored.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

Windows PowerShell is capable of detecting if the source code changed

There was very simple caching.
Current cache is very effective in most use cases. For example, we do not re-read the file every time, but just see if timestamp was changed. We don't cache based on source text but on SourceTree.

Are you saying the same can't be done in PowerShell Core?

No. Our thoughts was that the behavior is more safe in multi runspace environment. For example, we could destroy foregroud job by running a script which re-define a custom type. Recommendation for script/module authors is to define types in custom namespace.

quietly ignored.

There should be a verbose message.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

There should be a verbose message.

That's what this issue is asking for.

Note that it's not just about clashes between different authors. It's also about debugging by a single author, rerunning a script in a given session after having modified the source code, not realizing that the redefinition won't be possible in the same session - from what I understand, it's technically impossible to redefine a given .NET type in a session, because unloading assemblies isn't possible, right?

So we're in agreement, then? Your original response sounded like you favored the quiet ignoring.

@daxian-dbw

This comment has been minimized.

Copy link
Member

commented May 12, 2019

If you modify the source code, you get an error along the lines of:
Add-Type : Cannot add type. The type name '' already exists.

This should be the expected behavior. We should fix Add-Type in PS core.

subsequent calls are benign - and fast - no-ops, if the source code is unchanged.

Agreed that a verbose message should be added in this case.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Thanks, @daxian-dbw.

A message to the verbose stream is already being issued (but only in PS Core); i.e., you need to use -Verbose to see it: The source code was already compiled and loaded.

I think that's sufficient - or were you thinking the message should be displayed unconditionally?

I think it's better to be silent by default; this allows users not to have write additional logic around Add-Type calls: simply call them every time in your script, relying on subsequent calls to be quiet no-ops.

@fMichaleczek

This comment has been minimized.

Copy link

commented May 12, 2019

I already remove additional logic around Add-Type calls and i don't think I'm the only one.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

Note that it's not just about clashes between different authors. It's also about debugging by a single author, rerunning a script in a given session after having modified the source code, not realizing that the redefinition won't be possible in the same session - from what I understand, it's technically impossible to redefine a given .NET type in a session, because unloading assemblies isn't possible, right?

  1. Current behavior is nice for regular use.
  2. For development author have to re-run pwsh. I do so and don't think that it is a big problem.
  3. Add-Type compiles to new assembly and the new assembly is used then - so we could "re-define" types (in script at least). This can be useful in dev but dangerous in all other scenarios. If I wrong @daxian-dbw will correct me.
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

  1. For development author have to re-run pwsh. I do so and don't think that it is a big problem.

I too don't think that the need to re-start the session is a problem, and I'm not asking to change that.

I do, however, think that not alerting the user to the need for starting a new session is the problem, and it sounds @daxian-dbw agrees with that.

  1. Add-Type compiles to new assembly and the new assembly is used then - so we could "re-define" types (in script at least). This can be useful in dev but dangerous in all other scenarios. If I wrong @daxian-dbw will correct me.

It isn't dangerous. If the source code is effectively the same - and that would be the only scenario in which the quiet no-op (no error) occurs - then there's no problem, regardless of who calls Add-Type and how often.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

If the source code is effectively the same

I mean that only full type name is the same - that is dangerous.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

I mean that only full type name is the same - that is dangerous.

That's not what we're talking about here.
We're talking about the case where the source code hasn't changed for a given type.

Incidentally, the verbatim source-code caching that you believe is exclusive to Windows PowerShell seems to be in PowerShell Core as well, which I infer from the following:

The current PS Core code too knows the difference between the two scenarios (source code changed vs. unchanged), because the aforementioned verbose message is only emitted in the unchanged case; the changed case is completely silent.

Thus, it sounds like making the changed case emit a statement-terminating as in Windows PowerShell is all that is needed.

As an aside: I think that verbatim source-code caching as the basis for determining whether a type's definition has changed is sufficient, not least because it allows for much faster testing than having to compile every time. As stated, verbatim checking seems to be in the code already.

@mklement0 mklement0 changed the title Add-Type now quietly ignores attempts to redefine a .NET type in the same session Add-Type now quietly ignores attempts to change a previously added .NET type's definition in the same session May 12, 2019

@daxian-dbw

This comment has been minimized.

Copy link
Member

commented May 12, 2019

A message to the verbose stream is already being issued (but only in PS Core); i.e., you need to use -Verbose to see it

@mklement0 I didn't know the verbose message was already there. I agree that's sufficient.

I'm not sure if we are all clear on the current behavior as demonstrated in the repro steps:

Add-Type -TypeDefinition @'
  public class Foo {}
'@
  
# Try to redefine the type with different source code.
Add-Type -TypeDefinition @'
  public class Foo { public int Bar {get {return 42;} } }
'@

Today, the second Add-Type actually generates another assembly with the type Foo that contains a Bar property. However, the use of [Foo] subsequently will have undefined behavior -- it could be resolved to either the first [Foo] that has an empty body, or the second [Foo] with a Bar property, because the type resolution will stop after finding the first Foo from all loaded assemblies. See the example below:

> $as =[System.AppDomain]::CurrentDomain.GetAssemblies() | ? -not Location | select -Last 2
> $as.FullName
0pyel1t2.h2v, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
bycuxatk.jhq, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null

> $as[0].GetTypes()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     False    Foo                                      System.Object

> $as[1].GetTypes()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     False    Foo                                      System.Object

@iSazonov I don't think this is the right behavior for Add-Type, because there is no way for the user to reliably use the generated type. I think we should fix it to throw error when redefining the same type with different source code.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Thanks, @daxian-dbw; it sounds like we're in complete agreement (despite the fact that I hadn't realized that another assembly was actually being created, but that point is moot, given the suggested resolution).

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

I don't think this is the right behavior for Add-Type, because there is no way for the user to reliably use the generated type. I think we should fix it to throw error when redefining the same type with different source code.

Above I just discribe this and I was sure that this was already done as we already have CheckDuplicateTypes() and AllNamedTypeSymbolsVisitor

$a = Add-Type -TypeDefinition @'
   public class Foo {}
 '@ -PassThru

$a1 = Add-Type -TypeDefinition @'
   public class Foo {}
 '@ -PassThru

$a.Assembly.FullName
bwb1lxup.0m4, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null

$a1.Assembly.FullName
bwb1lxup.0m4, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I may have confused the issue by initially not realizing that a new assembly is created in the case of changed source code (resulting in a type with the same full name that isn't reliably used when referenced by that name).

So, are we all on the same page now, @iSazonov?

To recap, the desired behavior is:

  • If the source code didn't change, default to a fast and quiet no-op, although -Verbose allows emitting a message indicating that the existing definition is used - this part is already working.

  • If the source code did change, do not generate a new assembly and instead throw a statement-terminating error indicating that the type cannot be changed in-session - this is how it works in Windows PowerShell and how it should work in PowerShell Core as well.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Yes, code examples clarified and I prepared the fix.

@iSazonov iSazonov referenced a pull request that will close this issue May 15, 2019

Open

Block type update in Add-Type cmdlet #9609

8 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.