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

add tests for use domain formal before defined #26664

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

arezaii
Copy link
Collaborator

@arezaii arezaii commented Feb 6, 2025

Add regression test for a formal pattern that was previously allowing the use of formals before they were defined due to a bug/limitation in the compiler.

As an example, consider the following function signature:

proc foo(baz: t, type t) {
	//...
}

Here the formal baz is declared as having the type t, and type t is defined as the second formal.

The compiler rightly complains that we've used the type t before it was defined.

The solution is to reorder the formals such that the type t is defined first:

proc foo(type t, baz: t) {
	//...
}

However, because we can’t enforce domain type bounds at compile time, doing something similar with a domain appeared to work.

proc bar(baz: [d] real, d:domain(1)) {
	//...
}

This appeared to work because behind the scenes, the compiler is rewriting this array domain expression into the function body as an internal call due to special handling of array domain expressions.

As we move more of the resolution process into the new compiler (Dyno), errors like this are detected at compile time and so formals need to observe the same define-before-use rules as other variables.

This requires reordering the formals such that the domain comes before the array formal that uses it. If call sites do not use named arguments, their ordering will similarly need to be adjusted.

[reviewed by @mppf - thanks!]

@arezaii arezaii marked this pull request as ready for review February 7, 2025 22:32
Copy link
Member

@mppf mppf left a comment

Choose a reason for hiding this comment

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

Looks good. Ideally, these errors wouldn't talk about "statements" but rather "formals". But I know this PR is just adding a test.

@arezaii arezaii merged commit f6efe98 into chapel-lang:main Feb 11, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants