Add sequence literals #901
Conversation
This commit adds sequence literals with the syntax `{| elems+ |}`,
which enables assigning sequence literals to seq[T] variables.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for sequence literals to the P language with the new syntax {| elems+ |}, enabling developers to assign sequence literals directly to seq[T] variables.
- Add parser support for sequence literal syntax using
{|and|}delimiters - Implement type checking and AST representation for sequence literal expressions
- Include C# code generation for the new sequence literal feature
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SequenceLiteralExpr.p | Test file demonstrating sequence literal usage and validation |
| PParser.g4 | Grammar rules for sequence literal expressions and elements |
| PLexer.g4 | Lexical tokens for sequence literal delimiters |
| SequenceLiteralExpr.cs | AST node implementation for sequence literal expressions |
| ExprVisitor.cs | Type checker visitor method for sequence literals |
| DeclarationStubVisitor.cs | Stub visitor method for sequence literals |
| IRTransformer.cs | IR transformation logic for sequence literals |
| IrToPseudoP.cs | Debug output formatting for sequence literals |
| CSharpCodeGenerator.cs | C# code generation for sequence literal expressions |
| { | ||
| SourceLocation = sourceLocation; | ||
| SequenceElements = sequenceElements; | ||
| Type = new SequenceType(sequenceElements[0].Type); |
There was a problem hiding this comment.
This code will throw an IndexOutOfRangeException if sequenceElements is empty. The grammar allows empty sequences, but this constructor assumes at least one element exists.
| Type = new SequenceType(sequenceElements[0].Type); | |
| if (sequenceElements.Count > 0) | |
| { | |
| Type = new SequenceType(sequenceElements[0].Type); | |
| } | |
| else | |
| { | |
| // Assign a default type for empty sequences. Adjust as needed for your type system. | |
| Type = new SequenceType(PrimitiveType.Any); | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Good catch. Addressed as shown.
| | FLOAT LPAREN base=IntLiteral COMMA exp=IntLiteral RPAREN # ExpFloat | ||
| ; | ||
|
|
||
| seqElems : elems+=primitive |
There was a problem hiding this comment.
The grammar rule only allows 'primitive' expressions in sequence literals, but the test file and implementation suggest it should support any expression (like variables). This should be 'expr' instead of 'primitive' to match the test usage of variable 'a'.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
current code supports primitives and idents. Do we want to support the full range of expressions in the literals?
| break; | ||
|
|
||
| case SequenceLiteralExpr sequenceLiteralExpr: | ||
| context.Write(output, $"new {GetCSharpType(sequenceLiteralExpr.Type)}(new List<IPValue>"); |
There was a problem hiding this comment.
The generated C# code creates a List but should use the specific element type. For a seq[int], this should generate List or the appropriate typed collection, not List.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Not sure if this needs to be addressed. The generated code needs to a pass a List to the constructor for PSeq.
…type to empty literal
There was a problem hiding this comment.
While i like the general idea behind this PR, a couple of things to note:
- It conflicts both syntactically and terminologically with how sequences are defined in parameterized tests in the P3.0 branch. They don't need to be the same but they should be aligned or made much more distinct. For instance, parameterized test "sequences" can be of heterogeneous types; should these sequence literals support this?
- For P2.x, support should be added to the Java code generator.
- This will need additional work to support the PVerifier and PEx backends in P3.0.
| expr : primitive # PrimitiveExpr | ||
| | LPAREN unnamedTupleBody RPAREN # UnnamedTupleExpr | ||
| | LPAREN namedTupleBody RPAREN # NamedTupleExpr | ||
| | LSEQ seqElems RSEQ # SequenceLiteralExpr |
There was a problem hiding this comment.
This syntax conflicts with the other definition of sequence literals in the P3.0 branch (from the parameterized test work: https://github.com/p-org/P/blame/2febcebeb4bf16f333362280f2db01dd621a333f/Src/PCompiler/CompilerCore/Parser/PParser.g4#L281
We should either come up with a common representation and handling of sequences in code and parameterized tests.
| ; | ||
|
|
||
| seqElems : | ||
| | elems+=primitive (COMMA elems+=primitive)* |
There was a problem hiding this comment.
A trickiness with this use of primitive is that it doesn't handle negative numbers: negative numbers are not primitives but are unary operator expressions.
| { | ||
| entry | ||
| { | ||
| s1 = {| 1 , 2 , 3 |}; |
There was a problem hiding this comment.
Add a test for a negative int; I suspect it will fail.
| SourceLocation = sourceLocation; | ||
| SequenceElements = sequenceElements; | ||
|
|
||
| if (sequenceElements.Count > 0) { |
There was a problem hiding this comment.
Could we determine a more general type (like seq or seq) if the sequence elements are heterogeneous?
|
|
||
| // check whether all elements have the same type | ||
| if (elems.Count() > 0) { | ||
| var firstElementType = elems[0].Type; |
There was a problem hiding this comment.
Could we handle a more general type (like seq or seq) if the sequence elements are heterogeneous?
|
Thanks for the feedback! :) Will take a pass over the comments today and tomorrow and come up with some revisions. |
Wrote up a small PR to add sequence literals with the syntax
{| elems+ |}, which enables assigning sequence literals to seq[T] variables. Added tests and C# code generation. This is now valid syntax:Not sure if this feature is on the roadmap for the P team right now, feel free to close if required. Was just having fun exploring the P compiler.
Are there any tests for the Java backend? I haven't added the code generation for the Java backend, but could do that and add a few tests, if they exist.