Remove most use of "Bogus" from Microsoft.CSharp and fix remaining by JonHanna · Pull Request #22537 · dotnet/corefx

JonHanna · 2017-07-23T20:12:40Z

Remove concept of bogus from types and most symbols.

Since bogus types can't get as far as the dynamic binder, the only case where bogus can be true is when a non-indexer parametrized property, defined in a language where that is allowed, is encountered.

In other cases computing bogosity involves recursive checks that always hit false.

Give PropertySymbol a separate Bogus property to use where it could be set, and remove all other uses of GetBogus and similar.

Entails removal of ERR_BogusType, contributes to #22470

Identify indexer properties correctly in AddPropertyToSymbolTable

AddPropertyToSymbolTable assumes any property with parameters is an indexer, which makes the subsequent check that only indexers can have parameters tautologous.

Check DefaultMemberAttribute instead.

Remove useMethInstead from property symbols and consider always true.

useMethInstead should be set in AddPropertyToSymbolTable when bogus is set, but is not.

However, the only time it is looked for the only possible value is true as that is the only condition under which a symbol can be bogus, so remove it, and the path for it being false.

This now means that ErrorCode.ERR_BindToBogusProp1 or ErrorCode.ERR_BindToBogusProp2 will be used when appropriate (contributes to #22470).

Fixes #22534

Tests for accessing non-indexer parameter properties
Remove bogus check within PredefinedMembers

Predefined properties cannot be bogus.

OmarTawfik · 2017-07-26T01:22:03Z

        {
            Name name;
-            bool isIndexer = property.GetIndexParameters() != null && property.GetIndexParameters().Length != 0;
+            bool isIndexer = property.GetIndexParameters().Length != 0


Please use Any()

Any reason to use predicateless-Any() on an array? The perf difference isn't much, but it's there, and Length != 0 is common with arrays elsewhere in corefx.

I think Length is ok when we know it is an array. "Any" is often faster with various kind of collections/enumerables, but for array we know the "Length != 0" is the simplest way to check for emptiness.

In reply to: 129472003 [](ancestors = 129472003)

OmarTawfik · 2017-07-26T01:22:56Z

            Name name;
-            bool isIndexer = property.GetIndexParameters() != null && property.GetIndexParameters().Length != 0;
+            bool isIndexer = property.GetIndexParameters().Length != 0
+                             && (property.DeclaringType?.GetCustomAttribute(typeof(DefaultMemberAttribute)) as DefaultMemberAttribute)


DefaultMemberAttribute [](start = 83, length = 22)

GetCustomAttribute()?

Sorry, I don't understand this comment.

Oh. TIL about the generic GetCustomAttribute<>() extension!

OmarTawfik · 2017-07-26T01:27:57Z

@@ -288,46 +288,7 @@ typeThru is NullableType ||

        public bool CheckBogus(Symbol sym)


Please use this everywhere this functionality is needed. Example: GroupToArgsBinder.LookForCandidates()

Done. Have also made it static as it requires no state.

OmarTawfik · 2017-07-26T01:29:14Z

            return (IndexerSymbol)this;
        }
+
+        public bool Bogus { get; set; }


I suggest keeping with other fields, or converting all fields to properties.
I know there is a bigger effort of refactoring these things, but we shouldn't special case this here now.

isIndexer and AsIndexerSymbol should ideally have been changed when getting rid of the similar methods defined elsewhere, anyway.

OmarTawfik

Left comments.

OmarTawfik · 2017-07-26T01:43:02Z

        }
+
+        [Fact]
+        public void LiteralDoubleCast()


Not sure how this relates to this PR. Maybe from #22533?

Yeah, something slipped while doing a bunch of rebases. Sorry.

Since bogus types can't get as far as the dynamic binder, the only case where bogus can be true is when a non-indexer parametrized property, defined in a language where that is allowed, is encountered. In other cases computing bogosity involves recursive checks that always hit false. Give PropertySymbol a separate Bogus property to use where it could be set, and remove all other uses of GetBogus and similar. Entails removal of ERR_BogusType, contributes to #22470

AddPropertyToSymbolTable assumes any property with parameters is an indexer, which makes the subsequent check that only indexers can have parameters tautologous. Check DefaultMemberAttribute instead.

useMethInstead should be set in AddPropertyToSymbolTable when bogus is set, but is not. However, the only time it is looked for the only possible value is true as that is the only condition under which a symbol can be bogus, so remove it, and the path for it being false. This now means that ErrorCode.ERR_BindToBogusProp1 or ErrorCode.ERR_BindToBogusProp2 will be used when appropriate. Fixes #22534

Predefined properties cannot be bogus.

VSadov · 2017-07-27T20:03:19Z

        public bool isOverride;             // Overrides an inherited member. Only valid if isVirtual is set.
        // false implies that a new vtable slot is required for this method.
-        public bool useMethInstead;         // Only valid iff isBogus == TRUE && IsPropertySymbol().
        // If this is true then tell the user to call the accessors directly.


This comment refers to the code that is now gone. Also the comment above looks like after the code it is referring to. Can that be fixed too?

Good point. Done.

VSadov

LGTM

…otnet/corefx#22537) * Remove concept of bogus from types and most symbols. Since bogus types can't get as far as the dynamic binder, the only case where bogus can be true is when a non-indexer parametrized property, defined in a language where that is allowed, is encountered. In other cases computing bogosity involves recursive checks that always hit false. Give PropertySymbol a separate Bogus property to use where it could be set, and remove all other uses of GetBogus and similar. Entails removal of ERR_BogusType, contributes to dotnet/corefx#22470 * Identify indexer properties correctly in AddPropertyToSymbolTable AddPropertyToSymbolTable assumes any property with parameters is an indexer, which makes the subsequent check that only indexers can have parameters tautologous. Check DefaultMemberAttribute instead. * Remove useMethInstead from property symbols and consider always true. useMethInstead should be set in AddPropertyToSymbolTable when bogus is set, but is not. However, the only time it is looked for the only possible value is true as that is the only condition under which a symbol can be bogus, so remove it, and the path for it being false. This now means that ErrorCode.ERR_BindToBogusProp1 or ErrorCode.ERR_BindToBogusProp2 will be used when appropriate. Fixes dotnet/corefx#22534 * Tests for accessing non-indexer parameter properties * Remove bogus check within PredefinedMembers Predefined properties cannot be bogus. * Refactor CheckBogus to static and use consistently. * Replace IsIndexer and AsIndexer with direct operations * Make all fields in PropertySymbol auto properties. * Use generic form of GetCustomAttribute in looking up default member. * Remove confusing leftover remnant of comment. Commit migrated from dotnet/corefx@a2b8a66

dnfclas added the cla-already-signed label Jul 23, 2017

karelz assigned JonHanna Jul 24, 2017

karelz added the area-Microsoft.CSharp label Jul 24, 2017

karelz assigned VSadov and OmarTawfik Jul 24, 2017

JonHanna force-pushed the ms_cs_remove_bogus branch 4 times, most recently from b27f6f1 to ad797e1 Compare July 26, 2017 01:15

OmarTawfik reviewed Jul 26, 2017

View reviewed changes

JonHanna added 8 commits July 26, 2017 12:35

Identify indexer properties correctly in AddPropertyToSymbolTable

a25d7d4

AddPropertyToSymbolTable assumes any property with parameters is an indexer, which makes the subsequent check that only indexers can have parameters tautologous. Check DefaultMemberAttribute instead.

Tests for accessing non-indexer parameter properties

47d10ea

Remove bogus check within PredefinedMembers

5cdcc28

Predefined properties cannot be bogus.

Refactor CheckBogus to static and use consistently.

a0f2d2c

Replace IsIndexer and AsIndexer with direct operations

202dba5

Make all fields in PropertySymbol auto properties.

680e0ff

JonHanna force-pushed the ms_cs_remove_bogus branch from ad797e1 to 680e0ff Compare July 26, 2017 12:16

Use generic form of GetCustomAttribute in looking up default member.

c3221ec

VSadov reviewed Jul 27, 2017

View reviewed changes

Remove confusing leftover remnant of comment.

ca232cb

JonHanna force-pushed the ms_cs_remove_bogus branch from a4773d5 to ca232cb Compare July 27, 2017 20:16

JonHanna mentioned this pull request Jul 27, 2017

Microsoft.CSharp PredefinedTypes remove dead code and tidy up #22471

Merged

VSadov approved these changes Jul 27, 2017

View reviewed changes

VSadov merged commit a2b8a66 into dotnet:master Jul 27, 2017

JonHanna deleted the ms_cs_remove_bogus branch July 27, 2017 23:22

karelz modified the milestone: 2.1.0 Aug 14, 2017

JonHanna mentioned this pull request Jan 31, 2020

Remove checks for impossible conditions from Microsoft.CSharp dotnet/runtime#22868

Closed

64 tasks

		@@ -288,46 +288,7 @@ typeThru is NullableType \|\|

		public bool CheckBogus(Symbol sym)

Conversation

JonHanna commented Jul 23, 2017

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!

OmarTawfik left a comment

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!

VSadov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants