Skip to content

Commit

Permalink
Merge pull request #20188 from mppf/fix-userInsteadOfStandard
Browse files Browse the repository at this point in the history
Add workarounds to get userInsteadOfStandard working for now

We started seeing failures in userInsteadOfStandard in some configurations after #19884 due to the additional checking it enabled. At the same time, PR #19306 temporarily accepted this test no longer behaving as desired. This PR applies workarounds to both the compiler and the test to get it working again.

However, it's my opinion that this pattern is not worth supporting, as I describe in this comment added to the top of `userInsteadOfStandard/foo2.chpl` by this PR:
```

// This test replaces an automatically-included standard library file with
// a user one because AutoMath.chpl exists as a sibling to this file.

// While this test uses AutoMath, as of this writing, there are 4 automatic
// standard libraries: AutoMath, Errors, ChapelIO, and Types.
// A workaround in parser.cpp allows this replacing process for these
// except for Errors (because it runs into another workaround in the parser).
// See issue #19313 for a non-workaround proposal to address this.

// One challenge here is that there can only be one copy of a top-level
// module in a given compilation. That means that if this test uses
// ./AutoMath.chpl as the AutoMath module, the internal library will
// refer to it for things like min/max instead of the real one.

// A related challenge to that, and part of the issue with Errors, is that in
// order for this to work, the first use of AutoMath that the compiler seeks
// to resolve has to be this one (vs one coming from somewhere else, e.g. an
// already-parsed included module). That presents an ordering constraint that
// causes problems when doing incremental or separate compilation. This issue,
// combined with the limited number of modules to which this can apply, make
// it potentially not worthwhile to support this pattern.
```

Reviewed by @lydia-duncan - thanks!

- [x] full local testing
  • Loading branch information
mppf authored Jul 11, 2022
2 parents 04da6a5 + 2069bb5 commit 02ba456
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 2 deletions.
12 changes: 12 additions & 0 deletions compiler/parser/parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,18 @@ static void ensureRequiredStandardModulesAreParsed() {
}
}

// Allow automatically-included standard libraries to be overridden
// by a user module.
// See also test/modules/bradc/userInsteadOfStandard.
// TODO: use a better strategy than this workaround in the future,
// such as ideas proposed in issue #19313.
if (0 == strcmp(modName, "AutoMath") ||
0 == strcmp(modName, "Errors") ||
0 == strcmp(modName, "ChapelIO") ||
0 == strcmp(modName, "Types")) {
foundInt = foundInt || foundUsr;
}

// If we haven't found the standard version of the module,
// then we need to parse it
if (foundInt == false) {
Expand Down
41 changes: 41 additions & 0 deletions test/modules/bradc/userInsteadOfStandard/AutoMath.chpl
Original file line number Diff line number Diff line change
@@ -1,3 +1,44 @@
// these are needed to compile the standard library,
// since this module is replacing the standard AutoMath.

proc max(a: int, b: int) {
if a > b {
return a;
} else {
return b;
}
}
proc max(param a: int, param b: int) param {
if a > b {
return a;
} else {
return b;
}
}

proc min(a: int, b: int) {
if a < b {
return a;
} else {
return b;
}
}
proc min(param a: int, param b: int) param {
if a < b {
return a;
} else {
return b;
}
}

proc abs(a: int) {
if a < 0 {
return -a;
} else {
return a;
}
}

proc testmath() {
writeln("In my math!");
}
Expand Down
22 changes: 22 additions & 0 deletions test/modules/bradc/userInsteadOfStandard/foo2.chpl
Original file line number Diff line number Diff line change
@@ -1,3 +1,25 @@
// This test replaces an automatically-included standard library file with
// a user one because AutoMath.chpl exists as a sibling to this file.

// While this test uses AutoMath, as of this writing, there are 4 automatic
// standard libraries: AutoMath, Errors, ChapelIO, and Types.
// A workaround in parser.cpp allows this replacing process for these
// except for Errors (because it runs into another workaround in the parser).
// See issue #19313 for a non-workaround proposal to address this.

// One challenge here is that there can only be one copy of a top-level
// module in a given compilation. That means that if this test uses
// ./AutoMath.chpl as the AutoMath module, the internal library will
// refer to it for things like min/max instead of the real one.

// A related challenge to that, and part of the issue with Errors, is that in
// order for this to work, the first use of AutoMath that the compiler seeks
// to resolve has to be this one (vs one coming from somewhere else, e.g. an
// already-parsed included module). That presents an ordering constraint that
// causes problems when doing incremental or separate compilation. This issue,
// combined with the limited number of modules to which this can apply, make
// it potentially not worthwhile to support this pattern.

use AutoMath;

testmath();
Expand Down
4 changes: 2 additions & 2 deletions test/modules/bradc/userInsteadOfStandard/foo2.good
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
warning: Ambiguous module source file -- using ./AutoMath.chpl over $CHPL_HOME/modules/standard/AutoMath.chpl
foo2.chpl:3: error: unresolved call 'testmath()'
foo2.chpl:3: note: because no functions named testmath found in scope
In my math!
In my foo2

0 comments on commit 02ba456

Please sign in to comment.