Add back most CreateInstance APIs to AppDomain and Activator by MarcoRossignoli · Pull Request #30809 · dotnet/corefx

MarcoRossignoli · 2018-07-03T13:19:45Z

closes https://github.com/dotnet/corefx/issues/30190

Code was taken from https://referencesource.microsoft.com/

/cc @jkotas @danmosemsft @ericstj

…Domain.CreateInstanceFrom

MarcoRossignoli · 2018-07-03T13:22:23Z

src/System.Runtime/tests/System/ActivatorTests.cs

+        }
+
+        [Fact]        
+        static void CreateInstanceAssemblyResolve()


@jkotas with this test i wanted to cover https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Activator.cs#L132 but after coverage analysis seems it doesn't pass here, any thoughts?

I do not see anything obvious. It is possible that the assemblyFromResolveEvent path is unreachable code in CoreCLR if this does not hit it.

What do you think is bettere here?Do we need to go down coreclr bit to understand why path is unreachable?Or this tests is enough for the scope of CreateInstance?

I think you need to use invalid assembly name, like ,,,, to trigger the assembly resolve code path. AssemblyResolve event gets called for invalid names and can fix them up to something valid. For example, consider following program:

using System; using System.Reflection; class Program { static void Main(string[] args) { AppDomain.CurrentDomain.AssemblyResolve += CurrentDomain_AssemblyResolve; Console.WriteLine(Assembly.Load(",,,,")); } private static System.Reflection.Assembly CurrentDomain_AssemblyResolve(object sender, ResolveEventArgs args) { Console.WriteLine("AssemblyResolve: " + args.Name); return typeof(Program).Assembly; } }

It will print:

AssemblyResolve: ,,,, test, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null

I'll try and let you know! Thank's for tip!

@jkotas it works(as expected 😏)!Branch covered...the only uncovered branch is https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Activator.cs#L140 AssemblyContentType.WindowsRuntime is there a way to test WinRT assembly?

Type.GetType("Windows.Foundation.Collections.StringMap, Windows, Version=255.255.255.255, Culture=neutral, PublicKeyToken=null, ContentType=WindowsRuntime") returns WinRT type on .NET Core 2.1 today.

So I would expect that using Windows.Foundation.Collections.StringMap as type name and Windows, Version=255.255.255.255, Culture=neutral, PublicKeyToken=null, ContentType=WindowsRuntime as assembly name here should hit this path.

For PlatformDetection.IsNotWinRTSupported platforms, it should throw exception.

Ok WinRT branch covered.
The last two branch i can't trigger are:
https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Activator.cs#L154 i cannot return null from assemblyFromResolveEvent or assemblyString == null because throw exception.
https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Activator.cs#L167 i cannot trigger return null branch...maybe with some bindingAttr config...but i did't find anything so far(throw exceptions).

i cannot trigger return null branch

Agree. This branch looks like unreachable code. I think it should be fine to delete it.

Ok all branch of main method covered!

MarcoRossignoli · 2018-07-03T13:23:53Z

src/System.Runtime.Extensions/src/System/AppDomain.cs

+        public ObjectHandle CreateInstance(string assemblyName, string typeName)
+        {
+            // jit does not check for that, so we should do it ...
+            if (this == null)


@jkotas is this useful today?I read around was used for call/callvirt IL.

Right, it is fine to delete these checks.

MarcoRossignoli · 2018-07-03T13:49:55Z

D:\j\workspace\windows-TGrou---74aa877a\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Runtime.Remoting.ObjectHandle' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---74aa877a\src\System.Runtime\src\System.Runtime.csproj]

@danmosemsft @jkotas mmm...first time i get this error...with my local clr build/tests works well...what i'm missing?

ericstj · 2018-07-03T14:16:08Z

You need to make sure corefx has picked up the coreclr build where you added this type. Probably #30808

MarcoRossignoli · 2018-07-03T14:29:41Z

You need to make sure corefx has picked up the coreclr build where you added this type. Probably #30808

~~Sorry i don't understand...here is CI that fail...PR you linked could be already current clr bit used by CI self, isn't it?~~
Ahh..is open not merged...clear! Thank's a lot @ericstj!

ericstj · 2018-07-03T15:23:48Z

Looks like @stephentoub just merged that, so we can now rerun CI.
@dotnet-bot test this please

jkotas · 2018-07-03T17:25:01Z

src/System.Runtime.Extensions/src/System/AppDomain.cs

+
+            if (assemblyName == null)
+                throw new ArgumentNullException("assemblyName");
+            Contract.EndContractBlock();


You can delete these EndContractBlock annotations too.

jkotas · 2018-07-03T17:25:24Z

src/System.Runtime.Extensions/src/System/AppDomain.cs

+                throw new NullReferenceException();
+
+            if (assemblyName == null)
+                throw new ArgumentNullException("assemblyName");


Nit: We use nameof(assemblyName) for these

jkotas · 2018-07-03T17:26:39Z

src/System.Runtime/tests/TestLoadAssembly/ClassesSample.cs

@@ -0,0 +1,22 @@
+using System;


License header

jkotas · 2018-07-03T17:27:53Z

src/System.Runtime.Extensions/src/System.Runtime.Extensions.csproj

  </ItemGroup>
  <ItemGroup>
    <ProjectReference Include="..\..\System.Private.Uri\src\System.Private.Uri.csproj" />
+    <ProjectReference Include="..\..\System.Diagnostics.Contracts\src\System.Diagnostics.Contracts.csproj" />


This should not be needed once you cleanup the unnecessary annotations.

jkotas · 2018-07-05T02:35:01Z

src/System.Runtime/tests/System/ActivatorTests.cs

+
+        private static Assembly CurrentDomain_AssemblyResolve(object sender, ResolveEventArgs args)
+        {
+            return Assembly.LoadFile(Path.Combine(Directory.GetCurrentDirectory(), "TestLoadAssembly.dll"));


If other tests in this assembly happen to trigger AssemblyResolve event for unrelated assembly, this is going to break them. This AssemblyResolve event needs to be coded such that it is guaranteed to not affect other tests in the process, or this test needs to be launched in its own process.

jkotas · 2018-07-05T03:00:00Z

src/System.Runtime.Extensions/src/System/AppDomain.cs

+        {
+            ObjectHandle oh = CreateInstance(assemblyName, typeName);
+
+            return (oh != null) ? oh.Unwrap() : null;


Nit: These can be return oh?.Unwrap()

…onal operators

jkotas · 2018-07-05T09:53:18Z

src/System.Runtime/tests/System/ActivatorTests.cs

+        {
+            ObjectHandle oh = null;
+
+            if(exceptionType != null)


Nit: Space if (

i'm suspecting that some of you are bot, should be night in Redmond 😃

Anipik · 2018-07-13T05:47:43Z

src/System.Runtime/tests/System/ActivatorTests.netcoreapp.cs

+
+        public static IEnumerable<object[]> TestingCreateInstanceObjectHandleFullSignatureWinRT()
+        {
+            //string assemblyName, string typeName, bool ignoreCase, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes, returnedFullNameType


danmoseley · 2018-07-13T05:50:01Z

src/System.Runtime/tests/TestLoadAssembly/ClassesSample.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+


Nit double blank line

Anipik · 2018-07-13T05:54:21Z

src/System.Runtime.Extensions/src/System/AppDomain.cs

+        public ObjectHandle CreateInstance(string assemblyName, string typeName)
+        { 
+            if (assemblyName == null)
+                throw new ArgumentNullException(nameof(assemblyName));


can you please add the braces here to have uniformity with the other exceptions being thrown in this file

Anipik · 2018-07-13T05:56:03Z

src/System.Runtime.Extensions/src/System/AppDomain.cs

+        public object CreateInstanceAndUnwrap(string assemblyName, string typeName)
+        {
+            ObjectHandle oh = CreateInstance(assemblyName, typeName);
+


nit: extra line

i added extraline in all methods...remove all?

yes, it is neither accompanied by any type of brace nor its more than a couple of lines

is there a fixed rule for empty line? for example https://github.com/MarcoRossignoli/corefx/blob/ae8636faa74119668cdc605fd77a0a7361a31335/src/System.Runtime.Extensions/src/System/AppDomain.cs#L288 there isn't extraline

I dont remember if there is coding guideline for this. This is the general feedback i got earlier.

Anipik · 2018-07-13T06:00:20Z

src/System.Runtime.Extensions/tests/System/AppDomainTests.netcoreapp.cs

+                Assert.Equal(returnedFullNameType, oh.Unwrap().GetType().FullName);
+
+                obj = AppDomain.CurrentDomain.CreateInstanceAndUnwrap(assemblyName: assemblyName, typeName: type);
+                Assert.NotNull(obj);


can factor out these two lines into private function something like CheckValidity. will make the repetition less

Anipik · 2018-07-13T15:51:27Z

src/System.Runtime.Extensions/tests/System/AppDomainTests.netcoreapp.cs

+
+        public static IEnumerable<object[]> TestingCreateInstanceObjectHandleFullSignatureData()
+        {
+            //string assemblyName, string typeName, bool ignoreCase, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes, returnedFullNameType


add some space after //

Anipik · 2018-07-13T15:53:07Z

src/System.Runtime/tests/System/ActivatorTests.netcoreapp.cs

+
+        public static TheoryData<string, string, string, string, Type> TestingCreateInstanceFromObjectHandleData => new TheoryData<string, string, string, string, Type>()
+        {
+            //string physicalFileName, string assemblyFile, string typeName, returnedFullNameType, expectedException


space after //

Anipik · 2018-07-13T15:53:27Z

src/System.Runtime/tests/System/ActivatorTests.netcoreapp.cs

+
+        public static TheoryData<string, string, string, Type, bool> TestingCreateInstanceObjectHandleData => new TheoryData<string, string, string, Type, bool>()
+        {
+            //string assemblyName, string typeName, returnedFullNameType, expectedException


space after //

add space after all comments

sorry for all this commit I'm a little bit fried today 😵

MarcoRossignoli · 2018-07-22T18:24:18Z

@dotnet-bot test this please

danmoseley · 2018-07-24T03:01:18Z

@Anipik @jkotas any further feedback?

danmoseley · 2018-07-24T04:56:07Z

Let's give @jkotas a few days in case he is looking at github. There are subtleties to these API's that predate me so I want to get this right before we commit it.

…corefx#30809) Commit migrated from dotnet/corefx@ab6f3da

MarcoRossignoli added 10 commits June 30, 2018 11:19

add signature placeholder

965d74a

remove extralines

665e93f

move System.Runtime.Extensions from netfx

57d4f7c

fix using

fe65617

added tests Activator.CreateInstance, Activator.CreateInstanceFrom

47b5b23

added tests AppDomain.CurrentDomain.CreateInstance, AppDomain.Current…

596e5ce

…Domain.CreateInstanceFrom

fix visibility

47092fb

Merge branch 'master' into createinstanceapis

3772fce

code style cleanup

4db39d0

add test, cleanup code

209b019

MarcoRossignoli commented Jul 3, 2018

View reviewed changes

jkotas reviewed Jul 3, 2018

View reviewed changes

MarcoRossignoli added 5 commits July 4, 2018 11:18

address PR feedback

8d73059

resolve conflicts

632600c

nit: remove spaces

36c5cb8

fix tests for linux file system case sensitivity

ecfdcb8

amend test to cover assemblyFromResolveEvent branch

41ed67e

jkotas reviewed Jul 5, 2018

View reviewed changes

MarcoRossignoli added 2 commits July 5, 2018 11:38

added WinRT branch coverage test, use RemoteInvoker, use null-conditi…

43911df

…onal operators

nit: code alignment

f90d721

jkotas reviewed Jul 5, 2018

View reviewed changes

Anipik reviewed Jul 13, 2018

View reviewed changes

danmoseley reviewed Jul 13, 2018

View reviewed changes

Anipik reviewed Jul 13, 2018

View reviewed changes

MarcoRossignoli added 5 commits July 13, 2018 09:50

address PR feedback

89e5954

remove private accessor modifier

886bb50

address PR feedback

784298b

address PR feedback

2cc2435

address PR feedback

348f70f

Anipik reviewed Jul 13, 2018

View reviewed changes

MarcoRossignoli added 2 commits July 13, 2018 18:04

address PR feedback

4044672

address PR feedback

53ec45b

karelz added this to the 3.0 milestone Jul 19, 2018

karelz added the area-System.Runtime.Extensions label Jul 19, 2018

karelz assigned MarcoRossignoli and Anipik Jul 19, 2018

Anipik approved these changes Jul 24, 2018

View reviewed changes

danmoseley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 24, 2018

jkotas approved these changes Jul 24, 2018

View reviewed changes

jkotas removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 24, 2018

jkotas merged commit ab6f3da into dotnet:master Jul 24, 2018

MarcoRossignoli deleted the createinstanceapis branch July 24, 2018 06:00

ahsonkhan mentioned this pull request Nov 15, 2018

Merge relevant changes from the auto-generated System.Runtime ref #33512

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add back most CreateInstance APIs to AppDomain and Activator (dotnet/…

8fd6014

…corefx#30809) Commit migrated from dotnet/corefx@ab6f3da

Conversation

MarcoRossignoli commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoRossignoli Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli commented Jul 3, 2018

Uh oh!

ericstj commented Jul 3, 2018

Uh oh!

MarcoRossignoli commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MarcoRossignoli commented Jul 3, 2018 •

edited

Loading

MarcoRossignoli Jul 3, 2018 •

edited

Loading

MarcoRossignoli Jul 5, 2018 •

edited

Loading

MarcoRossignoli commented Jul 3, 2018 •

edited

Loading