Bugzilla – Bug 322762
[PATCH] Type.GetXxx(...) returns too many members
Last modified: 2008-01-16 19:43:07 UTC
---- Reported by juraj@hotfeet.ch 2006-11-27 08:09:32 MST ---- Type.GetFields(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance) must not return private fields declared in base types. The following output results from running the attached test case on Mono: ======== Type 'SomeClass' prot priv pub protBase privBase <=== this must not be listed! pubBase Type 'SomeBase' protBase privBase pubBase Type 'Object' Type 'SomeClass' prot priv pub Type 'SomeBase' protBase privBase pubBase Type 'Object' ========= While MS.NET produces: ========= Type 'SomeClass' prot priv pub protBase pubBase Type 'SomeBase' protBase privBase pubBase Type 'Object' Type 'SomeClass' prot priv pub Type 'SomeBase' protBase privBase pubBase Type 'Object' ========= I'm using Mono from SVN. The code at fault is in /mono/mono/metadata/icalls.c (ves_icall_Type_GetFields_internal). I haven't checked the other Type.Get{Properties,Events,Members,...} methods, they might suffer from the same problem. ---- Additional Comments From juraj@hotfeet.ch 2006-11-27 08:10:23 MST ---- Created an attachment (id=170976) test.cs - test case ---- Additional Comments From lupus@ximian.com 2006-11-28 07:02:19 MST ---- The GetFields() issue is fixed in svn. Would you mind writing the test cases and posting the output from the MS runtime of the other calls? Thanks. ---- Additional Comments From juraj@hotfeet.ch 2006-11-28 09:35:22 MST ---- Created an attachment (id=170977) get_tests.cs - test case (for fields, properties, events, methods and members) ---- Additional Comments From juraj@hotfeet.ch 2006-11-28 09:41:02 MST ---- Mono output for get_tests.cs ============================ Fields: Methods: get_privateBaseProperty add_privateBaseEvent remove_privateBaseEvent privateBaseMethod Finalize MemberwiseClone obj_address FieldGetter FieldSetter Properties: privateBaseProperty Events: privateBaseEvent Members: privateBaseEvent get_privateBaseProperty add_privateBaseEvent remove_privateBaseEvent privateBaseMethod Finalize MemberwiseClone obj_address FieldGetter FieldSetter privateBaseProperty MS.NET output ============= Fields: Methods: MemberwiseClone Finalize Properties: Events: Members: MemberwiseClone Finalize The mono output must not show any ...privateBase... lines. I'll turn the test case into a Nunit test for inclusion into corlib/Test/System/TypeTest.cs shortly. ---- Additional Comments From lupus@ximian.com 2006-11-28 11:11:01 MST ---- Thanks. Thinking about it more, we might want more comprehensive test cases. For example, what happens if a member is internal and the base class is in another assembly? If the call doesn't return private members it likely won't return that member as well. I think that the best way to go at this point is to write the test case in IL and test both with the base class in a separate assembly and in the same assembly of the derived type. IL is needed to be able to add fields with all the possible visibility values. Additionally for properties we could test what happens when the getter and setter have different visibility. There is also one additional codepath that needs checking: the Get methods that take a name. Note that nested types should be tested, too and constructors are handled in separate reflection calls, so they need adding to the tests as well. Sorry that this turned out to be much more work than initially thought. ---- Additional Comments From juraj@hotfeet.ch 2006-11-28 11:16:25 MST ---- Created an attachment (id=170978) patch to icall.c ---- Additional Comments From lupus@ximian.com 2006-11-28 11:28:34 MST ---- One more case for testing: when using a separate assembly we should likely also handle friend assemblies. ---- Additional Comments From juraj@hotfeet.ch 2007-07-19 10:40:53 MST ---- Just so I don't forget about it. The following command returns a listing of calls that would have to be checked before committing a fix: find mcs/class -name "*.cs" | grep -v "/Test/" | xargs grep BindingFlags.NonPublic Imported an attachment (id=170976) Imported an attachment (id=170977) Imported an attachment (id=170978) Unknown bug field "cf_op_sys_details" encountered while moving bug <cf_op_sys_details>FC6</cf_op_sys_details> Unknown operating system unknown. Setting to default OS "Other".
I did some tests for properties as part of bug #349078 (see gert/standalone/bug349078 in SVN for a standalone repro... you need NAnt to build it), with the following results: 1) properties with internal accessors in base classes are only included in .NET 2.0. 2) whether the (base) class is in the same assembly or a separate assembly has no effect. 3) the InternalVisibleToAttribute has no effect on reflection. 4) The behavior for nested types is identical. These results apply to GetProperties and GetProperty. I haven't yet done tests with accessors with different visibility. Which combinations should I check (and don't tell me "all" lol)?
The same results also apply for methods. For fields the story is the same, except that internal fields are also included on .NET 1.x (and not only on .NET 2.0). For constructors, only the ctors of the current type are returned. Standalone test case available here in SVN: gert/standalone/bug322762
Created attachment 188607 [details] Runtime patch (for icall.c) Modifies runtime implementation for Type.GetField(s) to match MS: if NonPublic flag is set, only return field(s) from parent if field is not private. Modifies runtime implementation of Type.GetMethod(s) to match MS: if NonPublic flag is set, do not include methods from parent class if they are private or - on the 1.0 profile - internal.
I've attached a patch which brings us inline with the MS behavior for fields and methods. For properties and constructors our implementation already matches that of MS. Left to do: 1) Fix our TypeBuilder implementation, and check for possible regressions that this may cause in (g)mcs. 2) Check our Type(Builder) implementation for GetEvent(s) and GetMember(s) against MS.
Created attachment 188917 [details] Updated runtime patch (for icall.c) * icall.c (ves_icall_Type_GetField): if NonPublic flag is set, only return field from parent class if not private. (ves_icall_Type_GetFields_internal): if NonPublic flag is set, only returns fields from parent class if they are not private. (method_nonpublic): added function to determine if a given method should be considered non-public. Returns false for private methods on parent class, and internal methods from parent on the 1.0 profile. (ves_icall_Type_GetMethodsByName): if NonPublic flag is set, then use method_nonpublic function to determine whether method should be returned. (property_accessor_public): use newly introduced method_nonpublic function to determine whether accessor is non-public. (ves_icall_MonoType_GetEvent): If NonPublic flag is set, only return event from parent class if not private. Only return static event if Static flag is set, and only return static event from parent class if FlattenHierarchy flag is set. (ves_icall_Type_GetEvents_internal): If NonPublic flag is set, only include non-private events from parent class.
Created attachment 188918 [details] corlib patch * TypeBuilder.cs (GetConstructorImpl): Use GetConstructor on created type. Original implementation did not take into account the binding flags and failed with a NotSupportedException if the default binder checked if a ParamArrayAttribute was defined. It also does not make sense to return a ConstructorBuilder once the type is emitted. (GetConstructors): When the type is emitted, use GetConstructors on the created type. (GetFields): Removed duplicate code. (GetMethodsByName): Fixed matching of methods in the parent class depending on their accessibility: - Private: never include private methods of parent - Public: ignore if Public flag is not set - Assembly: ignore if NonPublic is not set or when on the 1.0 profile - Rest (Family, FamANDAssem, FamORAssem): ignore if NonPublic flag is not set Static methods of the parent are ignored unless the FlattenHierarchy flag is set. (GetProperties): When the type is emitted, use GetProperties on the created type as the TypeBuilder implementation itself does not include properties from the parent class.
I've now attached an updated runtime patch which also fixes our runtime implemenation for Type.GetEvent(s) to match MS. The corlib patch I attached fixes the same issues for our TypeBuilder implementation. Both sets of changes are extensively covered by either unit tests or the standalone test I mentioned above (gert/standalone/bug322762). The patch passes the corlib and system unit tests (for 1.0 and 2.0 profile), and both the compiler and runtime tests. Is this ok to commit?
Please commit, thanks!
Fixed in SVN (revision 93079 for runtime, and 93080 for corlib part).