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

Generate a const #8

Open
3 of 7 tasks
adamconnelly opened this issue Aug 8, 2020 · 0 comments
Open
3 of 7 tasks

Generate a const #8

adamconnelly opened this issue Aug 8, 2020 · 0 comments
Assignees

Comments

@adamconnelly
Copy link
Owner

adamconnelly commented Aug 8, 2020

Thrift supports defining constants that will be generated into C#, and can also be referenced within the Thrift IDL. Here's an example:

const i32 MagicNumber = 123

struct Test {
  1: i32 Field = MagicNumber
}

This would generate something along these lines:

public static class Constants
{
    public const int MagicNumber = 123;
}

public class Test
{
    private int? _Field = Constants.MagicNumber;

    public int? Field
    {
        get
        {
            return _Field;
        }

        set
        {
            _Field = value;
        }
    }
}

The Apache compiler generates a file for the constants called <thrift-filename>.Constants.cs, so if we have a file called User.thrift, we'll get a User.Constants.cs file produced with a UserConstants class inside it.

At the moment we've taken the approach of generating all of the definitions in a single .thrift file into a single .cs file with the same name. So a User.thrift file will cause a User.cs file to be created, rather than creating files named based on the thrift definition names. We'll aim to continue with this approach, but use the Thrift filename to create the name of the constants class. If the filename is called Constants.thrift, we might want to provide a special case and avoid duplicating the word Constants.

So we might end up with the following class names:

  • User.thrift -> UserConstants.
  • Constants.thrift -> Constants.
  • My.File.thrift -> My_FileConstants.

Questions:

  • Do we want to be smart about casing (e.g. constants.thrift -> Constants, user.thrift -> UserConstants)?

Tasks:

  • Support int constants.
  • Support decimal constants.
  • Support bool constants.
  • Support string constants.
  • Support collection constants.
  • Support custom object constants.
  • Initialise from referenced constant.

Errors / warnings:

  • Invalid type conversion (string assigned to an int, etc).
  • Value outside allowed range for type.
  • Name missing.
  • Deprecation warning for declaring a constant using slist.

Notes:

  • The official compiler doesn't seem to check the range of the constant expression is valid, so allows const i8 MyI8 = 128, for example.
  • The official compiler accepts true, false, and integers for bool constants, where <= 0 is false and positive numbers are true.
@adamconnelly adamconnelly mentioned this issue Aug 17, 2020
15 tasks
@adamconnelly adamconnelly self-assigned this Dec 12, 2020
adamconnelly added a commit that referenced this issue Jan 2, 2021
- Added support for parsing and generating integer constants.
- At the moment there isn't any error handling, so it will generate invalid expressions (for example
  an i32 expression being assigned to an i8).
- At the moment there's a slight blurring between a _type definition_ vs a _type reference_. In
  order to fully support constants and checking that the expression is compatible with the type of
  the constant we'll need to sort that, but for now I've just fudged it enough to work by adding
  static members for each base type in `BaseType`.
- Added some sample Thrift files for constants.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 3, 2021
- I've renamed ConstantValue and all the related types to ConstantExpression to better match the
  parser grammar.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 3, 2021
I've added support for the following errors:

- Constant name missing.
- Constant expression missing.
- Equals operator missing.
- Constant expression has the wrong type.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Added support for generating `byte` constants. Previously it wasn't possible because we didn't
  allow `i8` constant expressions to be assigned to fields of type `byte`.
- Changed the C# type that gets generated for a Thrift `byte` from `byte` to `sbyte` to match the i8
  type. I've realised this is what the official compiler does.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Added support for generating `byte` constants. Previously it wasn't possible because we didn't
  allow `i8` constant expressions to be assigned to fields of type `byte`.
- Changed the C# type that gets generated for a Thrift `byte` from `byte` to `sbyte` to match the i8
  type. I've realised this is what the official compiler does.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Added a new error message if a constant is declared using the `binary` type. It's possible that
  binary is supported for constants, but I couldn't find any documentation about it, and I couldn't
  think how it would be initialised based on the supported constant expressions. The Apache compiler
  allows string literals to be assigned to `binary` constants, but the resulting code doesn't
  compile.
- Added an explanation of this behaviour along with the rationale to the compilation.md document.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Added the ability to declare and generate boolean constants.
- It turns out that Thrift supports `true`, `false`, and numeric values for initialising boolean
  constants. Negative numbers and `0` are false, and positive numbers are true.
- Added support for boolean constant expressions to the grammar, and moved the `IDENTIFIER`
  token below it to avoid them being parsed as identifiers.
- Created a `CSharpValue` property on the `Constant` class to allow us to convert to a valid C#
  initialization expression for numeric truthy values. The implementation is a little messy just now
  but I've just gone with it temporarily because I need to refactor the type system a bit as part of
  the constants work, and I think I can revisit it at that point.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Renamed `IConstant.Value` to `Expression` to better match the naming in the grammar.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Added a new error message if a constant is declared using the `binary` type. It's possible that
  binary is supported for constants, but I couldn't find any documentation about it, and I couldn't
  think how it would be initialised based on the supported constant expressions. The Apache compiler
  allows string literals to be assigned to `binary` constants, but the resulting code doesn't
  compile.
- Added an explanation of this behaviour along with the rationale to the compilation.md document.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Added the ability to declare and generate boolean constants.
- It turns out that Thrift supports `true`, `false`, and numeric values for initialising boolean
  constants. Negative numbers and `0` are false, and positive numbers are true.
- Added support for boolean constant expressions to the grammar, and moved the `IDENTIFIER`
  token below it to avoid them being parsed as identifiers.
- Created a `CSharpValue` property on the `Constant` class to allow us to convert to a valid C#
  initialization expression for numeric truthy values. The implementation is a little messy just now
  but I've just gone with it temporarily because I need to refactor the type system a bit as part of
  the constants work, and I think I can revisit it at that point.

Issues: #8
adamconnelly added a commit that referenced this issue Jan 9, 2021
- Renamed `IConstant.Value` to `Expression` to better match the naming in the grammar.

Issues: #8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant