Skip to content

detect "import submodule" and give proper errors#3215

Open
mtullsen wants to merge 4 commits intomasterfrom
tullsen/2618-import-submodule
Open

detect "import submodule" and give proper errors#3215
mtullsen wants to merge 4 commits intomasterfrom
tullsen/2618-import-submodule

Conversation

@mtullsen
Copy link
Copy Markdown
Contributor

@mtullsen mtullsen commented May 3, 2026

Plumb parsing data to the importCryptolModule so as to give proper error messages.
(Does not support the import submodule SUBMODULENAME yet)

@mtullsen mtullsen self-assigned this May 3, 2026
@mtullsen mtullsen marked this pull request as ready for review May 4, 2026 05:12
Copy link
Copy Markdown
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

It would be worth adding a test case to check that SAW is actually detecting import submodule as expected.

let import' = mkImport vis (locatedUnknown (T.mName mod')) as imps
return $ env' {eImports = import' : eImports env }
importCryptolModule _sc _env (Right __nm) _as True _vis _imps =
-- importing submodule by name:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth citing #2618 here in the comments, at least as a stopgap until #2618 is fully fixed.

Comment on lines -788 to +791
-- - `importCryptolModule`, which is the back end for SAWScript @import@
-- - `importCryptolModule`, which is the back end for SAWScript @import@
-- - `loadExtCryptolModule`, which is the back end for SAWScript @cryptol_load@
-- - `loadCryptolModule`, which is used for Rocq export and from crux-mir-comp
-- - `loadCryptolModule`, which is used for Rocq export and from crux-mir-comp
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The whitespace changes here appear unnecessary. I think there should only be a single space after each comma.

Comment on lines +115 to +128
import CryptolSAWCore.TypedTerm

import SAWCore.Name (nameInfo)
import SAWCore.Recognizer (asConstant)
import SAWCore.SharedTerm (NameInfo, SharedContext, Term, ppTerm)

import SAWSupport.Console
import qualified SAWSupport.Pretty as PPS

import qualified CryptolSAWCore.Cryptol as C
import CryptolSAWCore.Cryptol (ImportVisibility(..), CryptolEnv(..))
-- These used to live in this file, so import them unqualified for now.
-- XXX: tidy up

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you're going to fix up the import list, can you put them in structural order? (SAWSupport first, then SAWCore, then CryptolSAWCore)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, SAWSupport.Pretty is more fundamental than SAWSupport.Console so should really come before it.

importCryptolModule _sc _env (Left _) _as True _vis _imps =
-- importing submodule by FilePath: disallowed:
fail $ "`import submodule PATHNAME` is not allowed."
-- this allowed by parser?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is allowed by the parser. That could be changed, but TBH I think it's probably better to reject it explicitly than to generate "Parse error". We could, though, reject it in the SAWScript typechecker instead of here.

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.

3 participants