improve dyno scope resolve errors with lookup tracing #21637

mppf · 2023-02-19T13:52:57Z

This PR is intended to fix a failure with --dyno for the test

test/visibility/rename/renameUsedMod/renameModRenameSymBadAccess2.chpl

Previously, the dyno resolver was not causing an error, and then the production compiler's error handling did not print out the hint is used to, because the identifier Bar had been resolved to Foo by the dyno scope resolver.

This PR takes the following steps:

adds IllegalUseImport error and emit it in post-parse checks. The wording of this error message is migrated completely from the production compiler where it was added in PR Change internal error to user error for bad import expression #18731.
Renames the AmbiguousIdentifier error to AmbiguousVisibilityIdentifier and adds new errors UnknownIdentifier and AmbiguousIdentifier. Note that these two new errors are currently disabled for --dyno (but they are emitted when using the full dyno type and call resolver, e.g. in the dyno tests).
Adds a new NotInModule error for the case such as renameModRenameSymBadAccess2.chpl described above.
Adds lookupNameInScopeTracing which can record where matches came from for use in emitting errors
Adjusts class Scope to track if it contained an extern block (extern { }) so that further steps can be taken when looking for symbols in such a block.
Added LOOKUP_EXTERN_BLOCKS to enable search in extern blocks
Tests that now have different error message output (and will need .good file updates for --dyno):
Added CountIdentifiersWithName to support errors with parts like "first use this function" (the idea is that it's misleading to print that if the name is only used once in the function)
Added workaround code in Resolver.cpp to ignore results that match an extern block (since we have not yet implemented extern block support in the compiler library).
Adjusted name lookup functions to check extern blocks for a match after other things. This is draft code today & it doesn't yet have the ability to establish that a name is or is not provided by an extern block. But, symbols from an extern block should only be used if no other symbol with that name is present. So, for now it assumes that any name not found otherwise will resolve to an extern block (if present). This needs to be fixed in the future but it's OK for now because the Resolver has workaround code to ignore extern block matches.
Updates a few tests (including tests in the spec) that rely M.N.symbol where M contains a public use N. After PR simplify use/import shadowing #19306, public use should not bring in the module name. I've created issue bug: public use should not bring in module name for function call #21640 about how these cases are compiling in production when they should not.

NotInModule Example

module A {
  var x: int;
}

module Main {
  use A;
  proc main() {
    A.y;
  }
}

aa.chpl:7: In function 'main':
aa.chpl:8: error: cannot find 'y' in module 'A'

─── error in aa.chpl:8 [NotInModule] ───
  Cannot find 'y' in module 'A'
      |
    8 |     A.y;
      |     ⎺⎺⎺
      |

IllegalUseImport Example

(The wording of this error message is migrated completely from the production compiler where it was added in PR #18731.)

module Main {
  public import (if p then super.a else super.b) as A;
}

aa.chpl:1: In module 'Main':
aa.chpl:2: error: Illegal expression in 'import' statement
aa.chpl:2: note: only identifiers and 'dot' expressions are supported

─── error in aa.chpl:2 [IllegalUseImport] ───
  Illegal expression in 'import' statement
      |
    2 |   public import (if p then super.a else super.b) as A;
      |
  Only identifiers and 'dot' expressions are supported

UnknownIdentifier Example

(note, this error is currently disabled for --dyno as a workaround to problems unrelated to this PR)

module M {
  x;
}

aa.chpl:1: In module 'M':
aa.chpl:2: error: 'x' cannot be found

─── error in aa.chpl:2 [UnknownIdentifier] ───
  'x' cannot be found
      |
    2 |   x;
      |   ⎺
      |

AmbiguousIdentifier Example

(note, this error is currently disabled for --dyno as a workaround to problems unrelated to this PR)

module A {
  var x: int;
}
module B {
  var y: real;
}
module C {
  public import B.{y as x};
}

module M {
  use A, C;
  x;
}

aa.chpl:14: In module 'M':
aa.chpl:16: error: 'x' is ambiguous
aa.chpl:15: note: first, through the 'use' statement here
aa.chpl:2: note: found 'x' defined here
aa.chpl:15: note: additionally, through the 'use' statement here
aa.chpl:11: note: and then through the 'import' statement here
aa.chpl:8: note: and then through the 'import' statement providing 'y' here
aa.chpl:5: note: found 'z' defined here

─── error in aa.chpl:16 [AmbiguousIdentifier] ───
  'x' is ambiguous
       |
    16 |   x;
       |   ⎺
       |
  First, through the 'use' statement here:
       |
    15 |   use A, D;
       |       ⎺
       |
  Found 'x' defined here:
      |
    2 |   var x: int;
      |       ⎺⎺⎺⎺⎺⎺⎺
      |
  Additionally, through the 'use' statement here:
       |
    15 |   use A, D;
       |          ⎺
       |
  And then through the 'import' statement here:
       |
    11 |   public import C.{y as x};
       |                 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
       |
  And then through the 'import' statement providing 'y' here:
      |
    8 |   public import B.{z as y};
      |                 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
      |
  Found 'z' defined here:
      |
    5 |   var z: real;
      |       ⎺⎺⎺⎺⎺⎺⎺⎺
      |

module A {
  var x: int;
  var x: real;
}

module M {
  use A;
  proc main() {
    x;
  }
}

bb.chpl:9: error: 'x' is ambiguous
bb.chpl:7: note: through the 'use' statement here
bb.chpl:1: In module 'A':
bb.chpl:2: note: found 'x' defined here
bb.chpl:3: note: and found 'x' defined here

─── error in bb.chpl:9 [AmbiguousIdentifier] ───
  'x' is ambiguous
      |
    9 |     x;
      |     ⎺
      |
  Through the 'use' statement here:
      |
    7 |   use A;
      |       ⎺
      |
  Found 'x' defined here:
      |
    2 |   var x: int;
      |       ⎺⎺⎺⎺⎺⎺⎺
      |
  And found 'x' defined here:
      |
    3 |   var x: real;
      |       ⎺⎺⎺⎺⎺⎺⎺⎺
      |

Tests Needing .good Update for `--dyno`

After this PR, these tests will need .good file updates for --dyno:

modules/deitz/test_module_access1
modules/diten/modulescopes
visibility/import/edgeCases/dontLeakSymbols1
visibility/rename/badAccessWithPrefix

Future Work:

handle extern blocks in the dyno scope resolver / resolver (https://github.com/Cray/chapel-private/issues/3414)
get domain to scope resolve in dyno and enable the dyno identifier-not-found error for --dyno
change doLookupIn... functions to work with a struct instead of too many arguments.

Reviewed by @DanilaFe - thanks!

full local testing
no unexpected failures in --dyno testing

DanilaFe

Looking good, but I still haven't looked at scope-queries.cpp. Might be a little bit until I get to that, so publishing my comments so far.

DanilaFe · 2023-02-23T17:55:57Z

frontend/lib/framework/resolution-error-classes-list.cpp

+      } else {
+        msg += "it";
+      }
+      wr.note(ast, msg, " was provided by the automatically-included modules");


Notes and heading should have punctuation in their strings; it will be stripped in brief mode, and kept in detailed mode.

I'm not following what specifically you wanted me to change. Is it to add a period?

Yes, I think so.

DanilaFe · 2023-02-23T18:19:44Z

frontend/lib/framework/resolution-error-classes-list.cpp

+      wr.note(locationOnly(elt.visibilityClauseId), errbegin, " the '", elt.visibilityStmtKind, "' statement", nameSuffix, " here:");
+      wr.code<ID,ID>(elt.visibilityClauseId, { elt.visibilityClauseId });
+      from = elt.renameFrom;
+      first = false;


Could first be called something else, like switchModules, or something to indicate when it will / won't be true, and what that means? It would help better explain the code on like 876.

It's intended to literally reflect whether or not we are writing the first line of output, because if it is the first line we want to include intro. I'll add a comment to its declaration, though.

DanilaFe · 2023-02-23T18:27:03Z

frontend/lib/framework/resolution-error-classes-list.cpp

@@ -844,9 +951,82 @@ void ErrorAmbiguousIdentifier::write(ErrorWriterBase& wr) const {
    printedOne = true;
    wr.code<ID, ID>(id, { id });
  }
+
+  // TODO: call describeSymbolSource


Did you mean to leave this as a TODO for this PR?

No, I intended to leave it as future work.

DanilaFe · 2023-02-23T18:43:54Z

frontend/lib/resolution/Resolver.cpp

@@ -1233,6 +1233,41 @@ Resolver::issueErrorForFailedCallResolution(const uast::AstNode* astForErr,
  }
 }

+void Resolver::issueErrorForFailedModuleDot(const Dot* dot, ID moduleId) {
+  // figure out what name was used for the module in the Dot expression
+  auto modName = moduleId.symbolName(context);


Instead of reading the ID directly, could we use it to lookup the AST node or the module, and read the name from there? It seems more proper, at least to me because my mental model of IDs is that they are opaque unless absolutely necessary.

We will have more incremental reuse if we don't lookup the uAST from the ID. However in this case I don't think that the optimization is particularly key. Nonetheless I'd prefer to keep it the way it is.

DanilaFe · 2023-02-23T18:53:09Z

frontend/lib/resolution/Resolver.cpp

+      auto pair = namesWithErrorsEmitted.insert(ident->name());
+      if (pair.second) {
+        // insertion took place so emit the error
+        bool printFirstMention = identHasMoreMentions(ident);


Something makes me uneasy about a purely syntactic check like identHasMoreMentions. It says "more mentions this function", but what if I introduce a variable locally that shadows ident, and then reference it? that shouldn't count as another mention, right?

I'd also rename printFirstMention (imperative, "implementation detail" of the error) to mentionedMoreThanOnce (or just hasMoreMentions), which is a more "declarative" name.

One way to solve this "problem" might be to keep a map of identifier -> [unknown occurrence] in the Resolver , insert into it here, but defer reporting until the traversal is complete. The map can serve as a substitute for namesWithErrorsEmitted, because you'd just iterate the map's keys once. It would also help you forego the additional traversal.

The only trouble would be: "how do I know when to issue the errors?" Maybe the answer is: whenever a function is left?

I've renamed printFirstMention and added a TODO comment

frontend/test/resolution/testIf.cpp

and shift warnHiddenFormalsQuery to use it --- Signed-off-by: Michael Ferguson <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

For now, all names match the extern block. That is a TODO. This code currently uses workarounds to forget about the extern block resolution so that '--dyno' works with extern blocks. --- Signed-off-by: Michael Ferguson <[email protected]>

This is a belated follow-up to PR chapel-lang#19306. It is a bug that the production compiler allows this pattern today. --- Signed-off-by: Michael Ferguson <[email protected]>

Migrates the wording unchanged from PR chapel-lang#18731. For test/statements/errors/importIllegalExpr.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

otherwise it relies on a bug to compile --- Signed-off-by: Michael Ferguson <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

DanilaFe · 2023-02-23T22:04:38Z

frontend/lib/resolution/scope-queries.cpp

+static bool
+doLookupInImportsAndUses(Context* context,
+                         const Scope* scope,
+                         const ResolvedVisibilityScope* resolving,
+                         const ResolvedVisibilityScope* cur,
+                         UniqueString name,
+                         bool onlyInnermost,
+                         bool skipPrivateVisibilities,
+                         VisibilitySymbols::ShadowScope shadowScope,
+                         NamedScopeSet& checkedScopes,
+                         std::vector<BorrowedIdsWithName>& result,
+                         bool& foundExternBlock,
+                         std::vector<VisibilityTraceElt>* traceCurPath,
+                         std::vector<ResultVisibilityTrace>* traceResult) {


How many of these arguments do not change between recursive invocations? The list of args is making me think that we should have a wrapper struct that contains all the unchanging args, to avoid feeding them through every single function.

Some of the things that I think don't change and could be put in the struct:

Context

resolving

onlyInnermost

checkedScopes

result

foundExternBlock

traceCurPath

traceResult

I completely agree however I want to leave it as future work & the PR message already says this

Future Work:

...
change doLookupIn... functions to work with a struct instead of too many arguments.

DanilaFe · 2023-02-23T22:05:12Z

frontend/lib/resolution/scope-queries.cpp

+doLookupInAutoModules(Context* context,
+                      const Scope* scope,
+                      const ResolvedVisibilityScope* resolving,
+                      UniqueString name,
+                      bool onlyInnermost,
+                      bool skipPrivateVisibilities,
+                      NamedScopeSet& checkedScopes,
+                      std::vector<BorrowedIdsWithName>& result,
+                      bool& foundExternBlock,
+                      std::vector<VisibilityTraceElt>* traceCurPath,
+                      std::vector<ResultVisibilityTrace>* traceResult) {


This too would be a method on the struct I suggested in my previous comment (and so on)

DanilaFe · 2023-02-23T22:07:46Z

frontend/lib/resolution/scope-queries.cpp

+// for error messages. The expectation is that the common case is that
+// they are both nullptr.
+//
+// if traceCurPath is not nullptr, it will be updated to reflect the


Unless I misunderstand, you only ever push to traceCurPath if both traceCurPath and traceResult are non-null.

DanilaFe · 2023-02-23T22:17:37Z

frontend/lib/resolution/scope-queries.cpp

+
+  // If we found any extern blocks, and there were no other symbols,
+  // and extern block lookup was requested, use extern block lookup.
+  if (checkExternBlocks && startSize == result.size() && foundExternBlock) {


doesn't doLookupInScope return a found bool? Why not store it here / on line 1214 and then avoid the startSize == result.size() comparison/ even saving startSize?

--- Signed-off-by: Michael Ferguson <[email protected]>

@DanilaFe

This PR takes the following cleanup steps: * moves frontend/lib/framework/(parser-error-classes-list.cpp,resolution-error-classes-list.cpp} to the frontend/lib/parsing and frontend/lib/resolution directories, and similarly moves the headers * moves UnsupportedAsIdent to parser errors list since it is emitted by post-parse-checks * fix extra space in some error messages and avoids an unhelpful "In module" output for one error message (follow-up to PR #21637) * updates a number of loops like `for (auto elt : something())` to use `for (const auto& elt : something())` in cases where the element type is bigger than a pointer. The version without the `&` will cause a copy of the element on each iteration. Reviewed by @DanilaFe - thanks! - [x] full local testing - [x] no new `--dyno` failures

@arezaii

This is a follow-up to PR #21637. The dyno scope resolver's `doLookupIn...` functions have too many arguments and most of them do not change as these functions call each other. So, make them fields in a struct to make it easier to follow what is changing. Reviewed by @arezaii - thanks! - [x] full local testing - [x] no new failures with `--dyno`

PR chapel-lang#21637 added this error but it's currently off by default for --dyno so this test isn't testing a dyno error. --- Signed-off-by: Michael Ferguson <[email protected]>

mppf force-pushed the dyno-lookup-tracing branch from 46ec3f9 to 6704e2e Compare February 19, 2023 13:57

mppf mentioned this pull request Feb 21, 2023

bug: public use should not bring in module name for function call #21640

Open

mppf force-pushed the dyno-lookup-tracing branch from c5cccc1 to 2159e25 Compare February 21, 2023 15:42

mppf marked this pull request as ready for review February 21, 2023 20:45

DanilaFe self-requested a review February 22, 2023 20:45

DanilaFe reviewed Feb 23, 2023

View reviewed changes

mppf added 24 commits February 23, 2023 15:53

Add generalized tracing to doLookupInScope

7238bf0

and shift warnHiddenFormalsQuery to use it --- Signed-off-by: Michael Ferguson <[email protected]>

Move ResultVisibilityTrace to header

b5700b1

--- Signed-off-by: Michael Ferguson <[email protected]>

Add generalized "where did symbol come from" printing

2b4457b

--- Signed-off-by: Michael Ferguson <[email protected]>

Improve describeSymbolSource

9fe5045

--- Signed-off-by: Michael Ferguson <[email protected]>

Allow errors for identifiers not found

0c20c77

--- Signed-off-by: Michael Ferguson <[email protected]>

Track if it's 1st use of an ident or not

b16e6c5

--- Signed-off-by: Michael Ferguson <[email protected]>

Describe ambiguity

1c599b9

--- Signed-off-by: Michael Ferguson <[email protected]>

Add module dot expression error

f48c47a

--- Signed-off-by: Michael Ferguson <[email protected]>

Add rename location to error

4949c0a

--- Signed-off-by: Michael Ferguson <[email protected]>

Refactor error emitting code into a function

c70e89a

--- Signed-off-by: Michael Ferguson <[email protected]>

Temporarily turn off new errors

55477ca

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix unused variable GCC warning

3a5a508

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix modules primer for public use not bringing in mod name

e285915

This is a belated follow-up to PR chapel-lang#19306. It is a bug that the production compiler allows this pattern today. --- Signed-off-by: Michael Ferguson <[email protected]>

Add IllegalUseImport error

1290f58

Migrates the wording unchanged from PR chapel-lang#18731. For test/statements/errors/importIllegalExpr.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

Update test to use 'as' for public use

bc9c586

otherwise it relies on a bug to compile --- Signed-off-by: Michael Ferguson <[email protected]>

Add a future for pattern production accepts erroneously

e0d47ed

--- Signed-off-by: Michael Ferguson <[email protected]>

Update error message in .good file

bd3c402

--- Signed-off-by: Michael Ferguson <[email protected]>

Update spec tests for public use not enabling qualified

59b07a9

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix dyno linter error

d89f285

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix more dyno linter errors

fccb7a3

--- Signed-off-by: Michael Ferguson <[email protected]>

Improve error messages

2eacf9f

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix another dyno linter error

b8d7a94

--- Signed-off-by: Michael Ferguson <[email protected]>

Respond to review feedback

adbedf5

--- Signed-off-by: Michael Ferguson <[email protected]>

mppf force-pushed the dyno-lookup-tracing branch from 164f9f0 to adbedf5 Compare February 23, 2023 21:37

DanilaFe approved these changes Feb 23, 2023

View reviewed changes

Respond to more review feedback

2b2c00c

--- Signed-off-by: Michael Ferguson <[email protected]>

mppf merged commit 655c392 into chapel-lang:main Feb 23, 2023

mppf deleted the dyno-lookup-tracing branch February 23, 2023 23:08

mppf mentioned this pull request Feb 24, 2023

Compiler cleanups #21684

Merged

2 tasks

mppf mentioned this pull request Feb 25, 2023

Add helper struct for doLookupIn... #21695

Merged

2 tasks

mppf mentioned this pull request Mar 2, 2023

Improve multiply-defined symbol error in the dyno scope resolver #21734

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve dyno scope resolve errors with lookup tracing #21637

improve dyno scope resolve errors with lookup tracing #21637

mppf commented Feb 19, 2023 •

edited

Loading

DanilaFe left a comment

DanilaFe Feb 23, 2023

mppf Feb 23, 2023

DanilaFe Feb 23, 2023

DanilaFe Feb 23, 2023

mppf Feb 23, 2023

DanilaFe Feb 23, 2023

mppf Feb 23, 2023

DanilaFe Feb 23, 2023

mppf Feb 23, 2023

DanilaFe Feb 23, 2023

DanilaFe Feb 23, 2023

mppf Feb 23, 2023

DanilaFe Feb 23, 2023

mppf Feb 23, 2023

DanilaFe Feb 23, 2023 •

edited

Loading

DanilaFe Feb 23, 2023

DanilaFe Feb 23, 2023

improve dyno scope resolve errors with lookup tracing #21637

improve dyno scope resolve errors with lookup tracing #21637

Conversation

mppf commented Feb 19, 2023 • edited Loading

NotInModule Example

IllegalUseImport Example

UnknownIdentifier Example

AmbiguousIdentifier Example

Tests Needing .good Update for --dyno

DanilaFe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanilaFe Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf commented Feb 19, 2023 •

edited

Loading

Tests Needing .good Update for `--dyno`

DanilaFe Feb 23, 2023 •

edited

Loading