NotificationsYou must be signed in to change notification settings
Fork205
Star3.5k

Add sequence literals#901

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

anmolsahoo25 wants to merge2 commits intop-org:master

base:master

Choose a base branch

fromanmolsahoo25:feature-seq-literals

Open

Add sequence literals#901

anmolsahoo25 wants to merge2 commits intop-org:masterfromanmolsahoo25:feature-seq-literals

Conversation

Copy link

anmolsahoo25 commentedAug 10, 2025•
edited
Loading

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:

var s1 : seq[int]...s1 = {| 1,2,3 |}

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.

Add sequence literals

3c1e4a4

This commit adds sequence literals with the syntax `{| elems+ |}`,which enables assigning sequence literals to seq[T] variables.

ankushdesai requested review fromaman-goel,Copilot andlewisbru

August 11, 2025 20:22

CopilotAI reviewed

Aug 11, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 toseq[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

Src/PCompiler/CompilerCore/TypeChecker/AST/Expressions/SequenceLiteralExpr.cs Outdated

		{
		SourceLocation=sourceLocation;
		SequenceElements=sequenceElements;
		Type=newSequenceType(sequenceElements[0].Type);

Copy link

CopilotAIAug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This code will throw an IndexOutOfRangeException if sequenceElements is empty. The grammar allows empty sequences, but this constructor assumes at least one element exists.

Suggested change

	Type=newSequenceType(sequenceElements[0].Type);
	if(sequenceElements.Count>0)
	{
	Type=newSequenceType(sequenceElements[0].Type);
	}
	else
	{
	// Assign a default type for empty sequences. Adjust as needed for your type system.
	Type=newSequenceType(PrimitiveType.Any);
	}

Copilot uses AI. Check for mistakes.

Copy link

Author

anmolsahoo25Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good catch. Addressed as shown.

Src/PCompiler/CompilerCore/Parser/PParser.g4 Outdated

		\|FLOATLPAREN base=IntLiteralCOMMA exp=IntLiteralRPAREN # ExpFloat
		;

		seqElems : elems+=primitive

Copy link

CopilotAIAug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Author

anmolsahoo25Aug 11, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

current code supports primitives and idents. Do we want to support the full range of expressions in the literals?

Src/PCompiler/CompilerCore/Backend/CSharp/CSharpCodeGenerator.cs

		break;

		caseSequenceLiteralExprsequenceLiteralExpr:
		context.Write(output,$"new{GetCSharpType(sequenceLiteralExpr.Type)}(new List<IPValue>");

Copy link

CopilotAIAug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Author

anmolsahoo25Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not sure if this needs to be addressed. The generated code needs to a pass a List to the constructor for PSeq.

Added checks for all elements to be of same type and assign seq[any] …

798581b

…type to empty literal

lewisbru reviewed

Aug 12, 2025

View reviewed changes

Copy link

Contributor

lewisbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Src/PCompiler/CompilerCore/Parser/PParser.g4

		expr : primitive # PrimitiveExpr
		\|LPAREN unnamedTupleBodyRPAREN # UnnamedTupleExpr
		\|LPAREN namedTupleBodyRPAREN # NamedTupleExpr
		\|LSEQ seqElemsRSEQ # SequenceLiteralExpr

Copy link

Contributor

lewisbruAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Src/PCompiler/CompilerCore/Parser/PParser.g4

		;

		seqElems :
		\| elems+=primitive (COMMA elems+=primitive)*

Copy link

Contributor

lewisbruAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Tst/RegressionTests/Feature3Exprs/Correct/SequenceLiteralExpr/SequenceLiteralExpr.p

		{
		entry
		{
		s1= {\|1 ,2 ,3 \|};

Copy link

Contributor

lewisbruAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Add a test for a negative int; I suspect it will fail.

Src/PCompiler/CompilerCore/TypeChecker/AST/Expressions/SequenceLiteralExpr.cs

		SourceLocation=sourceLocation;
		SequenceElements=sequenceElements;

		if(sequenceElements.Count>0){

Copy link

Contributor

lewisbruAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could we determine a more general type (like seq or seq) if the sequence elements are heterogeneous?

Src/PCompiler/CompilerCore/TypeChecker/ExprVisitor.cs


		// check whether all elements have the same type
		if(elems.Count()>0){
		varfirstElementType=elems[0].Type;

Copy link

Contributor

lewisbruAug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could we handle a more general type (like seq or seq) if the sequence elements are heterogeneous?

Copy link

Author

anmolsahoo25 commentedAug 12, 2025

Thanks for the feedback! :) Will take a pass over the comments today and tomorrow and come up with some revisions.

Labels

None yet

Movatterモバイル変換

Add sequence literals#901

Are you sure you want to change the base?

Add sequence literals#901

Uh oh!

Conversation

anmolsahoo25 commentedAug 10, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

CopilotAIAug 11, 2025

Choose a reason for hiding this comment

Uh oh!

anmolsahoo25Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAIAug 11, 2025

Choose a reason for hiding this comment

Uh oh!

anmolsahoo25Aug 11, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CopilotAIAug 11, 2025

Choose a reason for hiding this comment

Uh oh!

anmolsahoo25Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

lewisbru left a comment

Choose a reason for hiding this comment

Uh oh!

lewisbruAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

lewisbruAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

lewisbruAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

lewisbruAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

lewisbruAug 12, 2025

Choose a reason for hiding this comment

Uh oh!

anmolsahoo25 commentedAug 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anmolsahoo25 commentedAug 10, 2025•
edited
Loading

anmolsahoo25Aug 11, 2025•
edited
Loading