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

Have the frontend library drive the process of parsing #25125

Merged
merged 99 commits into from
Jul 1, 2024

Conversation

mppf
Copy link
Member

@mppf mppf commented May 24, 2024

This PR adjusts the integration between the frontend library and the rest of the compiler. It changes it so that only live modules are converted from uAST to AST and so that this process occurs in the module initialization order. This is expected to reduce the number of fixups required.

There are a few user-facing changes in various edge cases:

  • This PR also changes how require "something.chpl"; interacts with the process of determining the main module & live modules. require statements no longer impact the determination of the main module & a module brought in by require will actually be dead-code-eliminated if it is not use or imported. See also issue How should 'require something.chpl' interact with finding the main module? #25112 for discussion of these changes.
  • This PR changes the behavior of test/modules/bradc/userInsteadOfStandard which is meant to show what happens if a user inadvertently creates a module with the same name as a standard module. This PR changes the ambiguous module chosen from ./ChapelIO.chpl to the standard library one and adds an unstable warning for such ambiguous module situations. See issue module search path ordering and name conflicts with bundled modules #25306 for discussion about what should happen in these cases.
  • The compiler now gives "surprising shadowing" warnings for cases where an identifier could refer to an enclosing module name or something brought in by a use statement.
  • In some cases, errors in modules that are not use or imported are no longer emitted. The reason for this is that these modules are eliminated earlier in compilation.
  • In some cases, the compiler will emit an error about not knowing what is the main module earlier in compilation before / in addition to other errors.

More detail about implementation changes in this PR:

  • This PR adjusts the dyno scope resolver to allow resolution of things like module M { M; } -- here M can refer to the enclosing module name even without use or import. This case was previously being handled by the production scope resolver.
  • This PR changes the way that certain built-in types are resolved. That was necessary to make the process more able to handle convert-uast being run on various modules in a different order from what the production compiler uses. Since we already had a list of 7 builtin types that needed to be wired up when their record/class declarations were found, I went ahead and moved this mechanism to something mediated by "well-known types" processing in wellknown.h / wellknown.cpp. Then, I added _tuple to the list of things that are processed in this way. Note that this involved moving several global variables for well-known types to wellknown.h / wellknown.cpp from various other locations.
  • Since I removed the global variable currentModuleType, pass the current module tag to buildManageStmt.
  • I moved the nestedName warning from checkUast.cpp to post-parse-checks.cpp (note that checkUast.cpp checks the production AST just after it was created by converting the uAST to it & shoul[d be renamed to check-generated-ast.cpp). Since this warning works with the ImplicitFileModule warning, I adjusted parseAndConvert.cpp to delay them both, so they can be issued together.
  • I added a global variable gConvertFilterModuleIds for the live module analysis to communicate to convert-uast.cpp which modules should be converted. Using a global variable in this way is not ideally and ideally this would be managed by having convert-uast.cpp only convert one module at a time.
  • Most of the changes are in parseAndConvert.cpp. Now the dyno code / frontend is in charge of deciding what to parse and convert and that process works with chpl::parsing::findMainAndCommandLineModules and chpl::resolution::moduleInitializationOrder to decide the order in which modules should be converted. I ran into some challenges where other parts of the compiler assume that certain modules appear early in allModules or that their DefExprs appear early in a traversal over theProgram. processInternalModules works around these issues by rearranging the key modules to appear in an order that matches what the rest of the compiler is used to.
  • I added some string utility functions (startsWith and replacePrefix) in a new string-utils.h / string-utils.cpp in the frontend library.
  • I added a Context::error & related methods to various Error handling classes to accept an IdOrLocation in order to wire up errors that indicate <command line> as the location for the error.
  • I added a libraryMode to findMainAndCommandLineModules to hide errors about the main module when creating a library. I also added some helper functions checkFileExists getExistingFileInDirectory getExistingFileInModuleSearchPath to support some operations using the module search path.
  • I adjusted Scope to track if it contains a require statement. This allows resolveVisibilityStmts to more accurately filter on which Scopes have meaningful work to do in order to resolve use/import/require.
  • I added deduplicateSamePaths to filesystem.h / filesystem.cpp that uses LLVM Support library functions to detect and remove paths that are redundant because they refer to the same filesystem element.
  • I adjusted findMainModuleImpl to consult the modules loaded as library files as a potential source for the main module. I also improved the error messages from this function.
  • I tidied up addCommandLineFileDirectories & adjusted it to put the command line file directories and -M paths before the paths from CHPL_MODULE_PATH.
  • I added doLookupEnclosingModuleName to implement lookup for cases like module M { M; }. I ran into problems with certain files named a keyword, so this uses isReservedIdentifier to ignore module name lookups for implicit modules with those names. It would be nice to adjust the lookup process to be more robust in this way, but this is the simple solution. See also some reserved words can't be used as a file name #19197 which tracks problems in this area.
  • I adjusted resolveVisibilityStmts process require with the module search directory if there are no /s in the path. I also adjusted it to process modules loaded by require. This addresses a pattern currently used in test/parsing/errors/nameLength where the main file requires another, which requires a third file; and then the main file uses a module expected to be made available by this nested require.

Reviewed by @DanilaFe - thanks!

  • full comm=none testing
  • full CHPL_COMM=gasnet testing

@mppf mppf force-pushed the dyno-convert-only-live branch 2 times, most recently from 6a02083 to ae655bc Compare June 27, 2024 13:07
@mppf mppf marked this pull request as ready for review June 27, 2024 13:22
@mppf mppf changed the title Convert only live modules Have the frontend library drive the process of parsing Jun 27, 2024
compiler/AST/build.cpp Outdated Show resolved Hide resolved
compiler/AST/wellknown.cpp Outdated Show resolved Hide resolved
compiler/AST/wellknown.cpp Show resolved Hide resolved
compiler/include/parser.h Outdated Show resolved Hide resolved
compiler/passes/normalize.cpp Outdated Show resolved Hide resolved
test/errors/parsing/unsupportedAsIdent2.chpl Show resolved Hide resolved
@@ -1 +0,0 @@
require "c-ray.chpl";
Copy link
Contributor

Choose a reason for hiding this comment

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

could you explain why you switched this to symlinks over requires?

Copy link
Member Author

Choose a reason for hiding this comment

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

require alone does not cause the modules brought in to not be dead (per the discussion in #25112). So, the old version would be equivalent to an empty source file (and do nothing useful).

Since c-ray.chpl creates the implicit module module c-ray which has a name that is not a valid identifier, I can't use c-ray;. Changing it to a symlink was the smallest change I could think of to get it working.

I don't think require is particularly relevant for what this test is doing. I think it was just a way to not use c-ray.chpl since it has an unfriendly name.

test/modules/lydia/inFunc.chpl Show resolved Hide resolved
mppf added 19 commits July 1, 2024 11:19
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
And include dtTuple in the process

This is to fix internal errors with

  test/types/tuple/bradc/tupleOfArray.chpl

caused by using dtTuple in build.cpp before it is initialized

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
By putting the work of checkBuilderResult into a query

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
This comes up with e.g.

  optimizations/sungeun/RADOpt/access2d

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added 16 commits July 1, 2024 11:19
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
For startsWith and replacePrefix

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
So that these tests can pass:
  performance/sungeun/multilocale/locale
  runtime/configMatters/comm/cache-remote/atomic

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
It should be OK for scopeForIdQuery to handle include statements because
it should use constructScopeQuery so as to avoid ending up with two
Scopes for the same ID

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
@mppf mppf merged commit 8386d95 into chapel-lang:main Jul 1, 2024
7 checks passed
@mppf mppf deleted the dyno-convert-only-live branch July 1, 2024 15:49
@mppf mppf mentioned this pull request Jul 1, 2024
mppf added a commit that referenced this pull request Jul 1, 2024
Follow-up to PR #25125 to remove an unused variable that causes problems
when building the compiler with `clang`.

Reviewed by @riftEmber - thanks!
mppf added a commit that referenced this pull request Jul 2, 2024
Follow-up to PR #25125.

Adjusts getExistingFileInModuleSearchPath to avoid issuing an ambiguity
warning for CHPL_LOCALE_MODEL=gpu where it is intentional that

    modules/internal/localeModels/gpu/ChapelGpuSupport.chpl

is used instead of the fallback

    modules/internal/ChapelGpuSupport.chpl

It does that by fixing getExistingFileInModuleSearchPath to avoid the
warning if both are bundled modules. However, for this to work in a
straightforward manner, I needed also to improve some of the related
code, which was not handling cases such as "." being a module search
path (because it was using prefix checks rather than something file-path
aware).

Reviewed by @dlongnecke-cray - thanks!

- [x] ambiguity warning no longer issued when compiling with
CHPL_LOCALE_MODEL=gpu
- [x] full comm=none testing
mppf added a commit that referenced this pull request Jul 3, 2024
Follow-up to PR #25125.

Resolves #23100.
Resolves #23129.

This PR adjusts `use` and `import` processing to make it case sensitive
even on non-case-sensitive filesystems.

`use M` or `import M` directs the compiler to go find `M.chpl` in the
module search path (assuming there is not already an `M` in scope). One
case-insensitive filesystems this presents a problem. On such
filesystems, file names have case, so `Foo.chpl` is different from
`foo.chpl`. However, you can open such a file with any case, so you
could open `Foo.chpl` with `FOO.CHPL` or `foo.chpl` or anything
in-between. This causes problems for the module search process:
* failures on some platforms when there is another file in a different
directory with different casing (see issue #23100)
 * tests are more sensitive to platform than we would like (see #23129)
* `import Library` can load `library.chpl` even though Chapel is
case-sensitive (see the test `test-import-wrong-case.chpl` added in this
PR). This can cause a confusing situation since Chapel can implicitly
create a module based on the file name, and this would be `Library` for
`import Library` but `library` for `import library` even though it uses
the same file.

This PR implements the case-sensitive handling for use/import by
matching file names against a directory listing of the containing
directory.

This PR also includes minor changes to address problems from PR #25125
when compiling with LLVM 11.

Reviewed by @dlongnecke-cray - thanks!

- [x] resolves ambiguity warning for primers tests on Mac OS X
- [x] `make && make check` works when using LLVM 11
- [x] full local comm=none testing
mppf added a commit that referenced this pull request Jul 8, 2024
Follow-up to PR #25125 to avoid a valid warning about surprising
shadowing.

Trivial and not reviewed.
mppf added a commit that referenced this pull request Jul 8, 2024
Follow-up to PR #25125.

That PR moved "same name as the implicit file module" warning to dyno,
so it's now present in the warning output for chpl-language-server.
Update the test to reflect that.

Reviewed by @DanilaFe - thanks!

- [x] make test-cls now passes
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.

None yet

3 participants