Allow user-specified underlying type for enums #8329

IISResetMe · 2018-11-21T20:16:37Z

PR Summary

This change adds support for specifiying the underlying type for an enum:

enum MyEnum : long 
{
  A = 0x0FFFFFFFFFFFFFFF
  B
}
# or
enum MyByte : byte 
{
  A = 0x01
  B = 0x02
  C = 0x03
  D
}

TODO:

re-introduce range validation (fixed in f417453)
- ~~currently breaks on negative value literals~~ (fixed in 19b2770)
better parsing errors for invalid underlying types (fixed in fc5a608)
~~value assignment is currently broken from non-int types~~ (fixed in f58eba8)
negative parsing tests

PR Checklist

This commit changes the behavior of the DefineEnumHelper, specifically the way the underlying type of the Enum type is specified. 06ff8a3 makes the parser recognize a base type token, this changes the underlying type of the generated EnumBuilder if the base type token corresponds to a builtin CLS value type as specified in ECMA-335 §I.8.5.2 (CLS Rule 7), otherwise it defaults to [int] TODO: - re-introduce range validation - better parsing errors for invalid underlying types

I'm thinking there must be an easier way of abstracting this away, but I can't think of anything

Forgot to reset the tokenizer in a previous commit, causing parsing errors.

Added new context for testing behavior when Flags() is applied. Increased test coverage for all underlying types, added type constraint test

enums can only be based off builtin integral types, parser should report an error if any other kind of type name is passed in

src/System.Management.Automation/engine/parser/PSType.cs

src/System.Management.Automation/resources/ParserStrings.resx

test/powershell/Language/Classes/scripting.enums.tests.ps1

Co-Authored-By: IISResetMe <IISResetMe@users.noreply.github.com>

Given that valid enum underlying types are all resolvable at parse-time (and unlikely to change), we might as well have the Parser error when an invalid underlying type is passed in as a type constraint.

…m/IISResetMe/PowerShell into feature/enum-with-underlying-type

ErrorId updated in @548b6cd3

src/System.Management.Automation/engine/parser/Parser.cs

Since (any valid) input value and maxValue are bound to be of the same type, we may as well mark maxValue dynamic as well - they will be comparable at runtime, and this avoids incorrectly erring when the user specifies a negative integer value for a member

src/System.Management.Automation/engine/parser/PSType.cs

src/System.Management.Automation/engine/parser/Parser.cs

src/System.Management.Automation/engine/parser/PSType.cs

src/System.Management.Automation/engine/parser/Parser.cs

src/System.Management.Automation/resources/ParserStrings.resx

test/powershell/Language/Classes/scripting.enums.tests.ps1

TravisEz13 · 2018-11-30T19:09:02Z

Can we get committee review for the new syntax?

SteveL-MSFT · 2018-12-05T23:47:50Z

@PowerShell/powershell-committee reviewed this and is fine with the syntax

adityapatwardhan · 2018-12-06T21:15:00Z

@TravisEz13 Please update your review.

stale · 2019-01-10T19:09:54Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

vexx32 · 2019-01-10T19:43:17Z

@TravisEz13 @IISResetMe apart from the merge conflict, was there anything else pending here? 💖

TravisEz13 · 2019-01-11T01:14:25Z

@vexx32 My comments have not been addressed. #8329 (review)

The contexts in which this error string is used covers not only literals that are too large, but also sometimes too small Renamed to EnumeratorValueOutOfBounds

IISResetMe · 2019-01-12T23:23:21Z

@TravisEz13 I believe the last two commits address your open comments :)

iSazonov · 2019-01-13T10:33:17Z

@IISResetMe Please resolve merge conflicts.

iSazonov · 2019-01-13T10:35:58Z

src/System.Management.Automation/engine/parser/Parser.cs

            //G  enum-member-list:
            //G      enum-member  new-lines:opt
            //G      enum-member-list   enum-member

+            const TypeCode validUnderlyingTypeCodes = TypeCode.Byte | TypeCode.Int16 | TypeCode.Int32 | TypeCode.Int64 | TypeCode.SByte | TypeCode.UInt16 | TypeCode.UInt32 | TypeCode.UInt64;


Const should be upper case:

const TypeCode ValidUnderlyingTypeCodes

iSazonov · 2019-01-13T10:37:17Z

src/System.Management.Automation/engine/parser/Parser.cs

+                var enumDefn = new TypeDefinitionAst(extent, name.Value, customAttributes == null ? null : customAttributes.OfType<AttributeAst>(), members, TypeAttributes.Enum, underlyingTypeConstraint == null ? null : new[] { underlyingTypeConstraint });
+                if (customAttributes != null && customAttributes.OfType<TypeConstraintAst>().Any())
+                {
+                    //no need to report error since there is error reported in method StatementRule


Please add space after //:

Suggested change

//no need to report error since there is error reported in method StatementRule

// No need to report error since there is error reported in method StatementRule.

IISResetMe · 2019-01-13T11:50:51Z

@iSazonov done!

IISResetMe · 2019-01-13T12:03:37Z

The Codacy issue currently failing is not really relevant.

While the compiler can't prove it, that path is unreachable since case TypeCode.Int32 will always be chosen if typeConstraintAst == null

iSazonov · 2019-01-13T17:28:48Z

Yes, I see. I see that Codacy has many false-positives.

iSazonov · 2019-01-18T04:15:38Z

@IISResetMe Thanks for your contribution!

IISResetMe added 3 commits November 21, 2018 20:47

Add basic enum basetype tests

88ce274

Add optional base type to enum parsing rule

06ff8a3

IISResetMe requested review from BrucePay and daxian-dbw as code owners November 21, 2018 20:16

IISResetMe added 10 commits November 21, 2018 21:48

Add support for Flags() and fix implied member literal values

f58eba8

Fix CodeFactor issue (SA1116)

c1bc80d

Add type-aware range check for enum member values

f417453

I'm thinking there must be an easier way of abstracting this away, but I can't think of anything

Reset tokenizer mode in EnumDefinitionRule

5d1ff8a

Forgot to reset the tokenizer in a previous commit, causing parsing errors.

Update error strings, add null check for Enum member values

fc5a608

Fix CodeFactor issue (SA1000, SA1019)

48ac5b3

Fix valueTooBig test (off-by-one) in DefineEnum()

55ee223

Fix type names in enum test

661e7c7

Add more enum tests

84edbf9

Added new context for testing behavior when Flags() is applied. Increased test coverage for all underlying types, added type constraint test

Better error handling + parser errors for invalid enum underlying type

d4486d8

enums can only be based off builtin integral types, parser should report an error if any other kind of type name is passed in

iSazonov reviewed Nov 22, 2018

View reviewed changes

iSazonov and others added 4 commits November 22, 2018 13:29

Optimize counter branch when defining enum member values

b0371ba

Co-Authored-By: IISResetMe <IISResetMe@users.noreply.github.com>

Add enum type constraint check to parser

dd6eee9

Given that valid enum underlying types are all resolvable at parse-time (and unlikely to change), we might as well have the Parser error when an invalid underlying type is passed in as a type constraint.

Merge branch 'feature/enum-with-underlying-type' of https://github.co…

548b6cd

…m/IISResetMe/PowerShell into feature/enum-with-underlying-type

Update enum type constraint test

1c767a9

ErrorId updated in @548b6cd3

iSazonov reviewed Nov 22, 2018

View reviewed changes

src/System.Management.Automation/engine/parser/Parser.cs Outdated Show resolved Hide resolved

IISResetMe added 3 commits November 22, 2018 14:45

Remove redundant enum type check from PSType

c43da74

Clean up DefineEnum()

19b2770

Since (any valid) input value and maxValue are bound to be of the same type, we may as well mark maxValue dynamic as well - they will be comparable at runtime, and this avoids incorrectly erring when the user specifies a negative integer value for a member

Fix CodeFactor issues (SA1116, SA1001)

d7a90a2

IISResetMe commented Nov 22, 2018

View reviewed changes

vexx32 reviewed Nov 22, 2018

View reviewed changes

src/System.Management.Automation/engine/parser/PSType.cs Outdated Show resolved Hide resolved

iSazonov reviewed Nov 23, 2018

View reviewed changes

IISResetMe added 3 commits November 23, 2018 23:10

Add enum negative value tests

ba4ccac

Refactor enum underlying type check

46c654a

Change enum type parser check to TypeCode

c6b2951

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 29, 2018

TravisEz13 added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Nov 30, 2018

TravisEz13 requested changes Nov 30, 2018

View reviewed changes

test/powershell/Language/Classes/scripting.enums.tests.ps1 Show resolved Hide resolved

TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 30, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Dec 5, 2018

stale bot added the Stale label Jan 10, 2019

stale bot removed the Stale label Jan 10, 2019

IISResetMe added 2 commits January 13, 2019 00:07

Rename ParserStrings.EnumeratorValueTooLarge

6d283d3

The contexts in which this error string is used covers not only literals that are too large, but also sometimes too small Renamed to EnumeratorValueOutOfBounds

Add test for enum literal value too small

80e119c

iSazonov reviewed Jan 13, 2019

View reviewed changes

IISResetMe and others added 3 commits January 13, 2019 12:29

Fix comment + variable casing

065e151

Merge branch 'master' into feature/enum-with-underlying-type

456d30b

Fix variable casing for ValidUnderlyingTypeCodes

85c36a8

TravisEz13 approved these changes Jan 17, 2019

View reviewed changes

TravisEz13 merged commit 59a3696 into PowerShell:master Jan 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user-specified underlying type for enums #8329

Allow user-specified underlying type for enums #8329

IISResetMe commented Nov 21, 2018 •

edited

Loading

TravisEz13 commented Nov 30, 2018

SteveL-MSFT commented Dec 5, 2018

adityapatwardhan commented Dec 6, 2018

stale bot commented Jan 10, 2019

vexx32 commented Jan 10, 2019

TravisEz13 commented Jan 11, 2019

IISResetMe commented Jan 12, 2019

iSazonov commented Jan 13, 2019

iSazonov Jan 13, 2019 •

edited

Loading

iSazonov Jan 13, 2019 •

edited

Loading

IISResetMe commented Jan 13, 2019

IISResetMe commented Jan 13, 2019

iSazonov commented Jan 13, 2019

iSazonov commented Jan 18, 2019

	//no need to report error since there is error reported in method StatementRule
	// No need to report error since there is error reported in method StatementRule.

Allow user-specified underlying type for enums #8329

Allow user-specified underlying type for enums #8329

Conversation

IISResetMe commented Nov 21, 2018 • edited Loading

PR Summary

PR Checklist

TravisEz13 commented Nov 30, 2018

SteveL-MSFT commented Dec 5, 2018

adityapatwardhan commented Dec 6, 2018

stale bot commented Jan 10, 2019

vexx32 commented Jan 10, 2019

TravisEz13 commented Jan 11, 2019

IISResetMe commented Jan 12, 2019

iSazonov commented Jan 13, 2019

iSazonov Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

iSazonov Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

IISResetMe commented Jan 13, 2019

IISResetMe commented Jan 13, 2019

iSazonov commented Jan 13, 2019

iSazonov commented Jan 18, 2019

IISResetMe commented Nov 21, 2018 •

edited

Loading

iSazonov Jan 13, 2019 •

edited

Loading

iSazonov Jan 13, 2019 •

edited

Loading