Add shift operations for numeric values

[abh80]

Added:

Bit shift operations to numeric values

constant amount = 2
constant value = 8
constant shifted = value << amount  # shifted=22

Closes #922

Tasks

Implementation

• Token.scala: added LSHIFT and RSHIFT tokens.
• Lexer.scala: added token parsing for new tokens.
• Ast.scala:
  • added new trait ShiftType to encapsulate shift types LShift extends ShiftType .....
  • added case class Shift(op: ShiftType) extends Binop to Binop.
• Parser.scala:
  • added expr binop node creation for new shift tokens.
  • implemented the parsing for shift tokens.
  • implement shift operand parsing with least precedence.
• analysis/CheckSemantics/CheckExprTypes.scala:
  • added a new helper method convertToInteger for shift operations (ignoring float values).
  • add the new case Ast.Binop.Shift(_).
• Value.scala:
  • added a new type intShiftOp = (BigInt, Int) => BigInt for shifting.
  • added a new private[analysis] def intShiftOp(op: Value.Binop.intShiftOp)(v: Value) (since generic binop does not support (BigInt, Int) => Bigint))
  • added two new methods to Value: isNegative and isValidShiftAmount to help in Analysis.scala.
  • implement the intShiftOp method in PrimitiveInt, Integer and EnumConstant.
  • added << and >> methods to Value to perform shift operations.
• Analysis.scala: added lshift and rshift method, using the new methods added to Value.
• CheckSemantics/EvalConstantExprs.scala: add the new case Ast.Binop.Shift(op) using the new methods added to Analysis.
• util/Error.scala: Added a new InvalidShiftAmount error to SemanticError.

Tests

• ‎compiler/tools/fpp-check/test/numeric_shift: added robust test cases including edge cases and error checks.
• compiler/lib/src/test/input/syntax/lexer/ok/symbols.fpp: added the new <</>> symbols.
• should reject float-operands.
• precedence check (1 + 2 << 3 parses as and shift operations must have lowest precedence).
• should reject negative-shift values.
• fpp-syntax tests.
• fpp-to-cpp tests.
• ValueSpec tests.

Spec:

• Update 3.2 symbols list
• Update the 10.1 arithmetic expressions table
• Update the 10.13. Precedence and Associativity
• Update the 18.12. Binary Arithmetic Expressions with new examples for this feature

have to address this: #995 (comment)

[abh80]

  /** For shifting operations make sure both values are integer */
  private def convertToInteger(loc: Location, t: Type): Result.Result[Type] =
    if (t.isInt) Right(t)
    else if (t.isConvertibleTo(Type.Integer)) Right(Type.Integer)
    else {                                                                                                                                                                                      
      val error = SemanticError.InvalidType(loc, s"cannot convert $t to an integer type")
      Left(error)                                                                                                                                                                               
    } 

The current implementation of int value check is too much permissive, since float is convertible to int. I will completely remove the conditional check. This causes the failure: https://github.com/abh80/fpp/blob/564f25468d2be477626cae34c49a0d5d354327f5/compiler/tools/fpp-check/test/numeric_shift/shift_float_value_error.ref.txt#L1

Edit: Looks like there is another bug

 t <- a.commonType(e.e1.id, e.e2.id, loc)                                                                                 
  _ <- e.op match {
    case Ast.Binop.Shift(_) => convertToInteger(loc, t)                                                                    
    ...
  }

The common type inference automatically promotes to Float even if one value is int which causes misleading error information. However this dosent affect the intended behavior since it's only applicable if both values are integer

[abh80]

@bocchino I have completed the implementation and testing. Can you give the initial review before I start writing the documentation?

[bocchino]

This generally looks good. I need to see a spec before I can approve it. Can you make a separate PR to the feature branch with just the spec changes? I'll review and approve, and then we can finalize this branch.

Some notes on the spec changes:

1. I think it's fine to add << and >> as new binary operations, but the behavior is a bit different from the other binary operations. In particular, I don't think it makes sense to compute the common type of the left and right operands. I think we should just require that each operand have an integer type. So we'll need to handle << and >> as separate cases from +, -, *, and /.
2. The precedence of << and >> is tricky. I think we should add a precedence table that shows the precedence and associativity of all the operators.
3. Remember to update the type checking rules. More generally, you can look at the diff of the string concatenation PR to see all the places that the spec needs to be updated.

[abh80]

1. I think it's fine to add << and >> as new binary operations, but the behavior is a bit different from the other binary operations. In particular, I don't think it makes sense to compute the common type of the left and right operands. I think we should just require that each operand have an integer type. So we'll need to handle << and >> as separate cases from +, -, *, and /.

This is a direct solution to the issue I found

The common type inference automatically promotes to Float even if one value is int which causes misleading error information. However this dosent affect the intended behavior since it's only applicable if both values are integer

Will add another case for this

[abh80]

I will open another spec pull request 👍

[abh80]

I don't think it makes sense to compute the common type of the left and right operands. I think we should just require that each operand have an integer type. So we'll need to handle << and >> as separate cases from +, -, *, and /.

Fixed this and opened a new pull request for spec revision #1000

[abh80]

@bocchino I have updated the implementation to include enum according to the spec. Can you give the review. Thanks