-
Notifications
You must be signed in to change notification settings - Fork 23
Changes to enable communication with JVerify #308
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
base: main
Are you sure you want to change the base?
Conversation
- Use writeJavaFiles helper in Test 11 (keyboardDrummer) - Document Test 12's dependency on comprehensive.ion (keyboardDrummer) - Fail if javac not found instead of skip (keyboardDrummer) - Use panic! for malformed SyntaxCat (joehendrix) - Add doc comments to GeneratedFiles and NameAssignments (joehendrix) - Collect warnings for unsupported type/function declarations (joehendrix)
PR strata-org#293 added dots to valid identifier characters, which broke qualified name resolution (e.g., 'Simple.Expr' was parsed as one identifier instead of dialect 'Simple' + name 'Expr'). Fix: In translateQualifiedIdent, split dotted identifiers on the first dot to extract dialect and name components. Also strip guillemets that the parser adds around special identifiers.
… lineoffsets when parsing Ion
- Fail on Init.TypeExpr (internal DDM machinery) - Fail on type/function declarations instead of warning - Remove warnings mechanism from GeneratedFiles - Fix cross-dialect category collision (A.Num vs B.Num) - Track QualifiedIdent instead of String for refs - Prefix with dialect name when names collide - Replace gradle with plain javac/java in regenerate script - Add test for cross-dialect name collision
| | file (path: String) | ||
| deriving DecidableEq, Repr | ||
|
|
||
| instance : ToFormat Uri where | ||
| format fr := match fr with | .file path => path | ||
|
|
||
| structure FileRange where | ||
| file: Uri | ||
| range: Strata.SourceRange | ||
| deriving DecidableEq, Repr | ||
|
|
||
| instance : ToFormat FileRange where | ||
| format fr := f!"{fr.file}:{fr.range}" | ||
|
|
||
| structure File2dRange where | ||
| file: Uri | ||
| start: Lean.Position | ||
| ending: Lean.Position | ||
| deriving DecidableEq, Repr | ||
|
|
||
| instance : ToFormat File2dRange where | ||
| format fr := | ||
| let baseName := match fr.file with | ||
| | .file path => (path.splitToList (· == '/')).getLast! | ||
| f!"{baseName}({fr.start.line}, {fr.start.column})-({fr.ending.line}, {fr.ending.column})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not appear in between the definition of SourceRange and the methods in it's namespace.
Strata/DDM/Ion.lean
Outdated
| | .mk (.list args) => | ||
| return .mk args (by simp; omega) | ||
| | _ => throw s!"Expected list" | ||
| | x => throw s!"Expected list but got {repr x}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values could be large. Repr may generate too much content.
Strata/DDM/Ion.lean
Outdated
| match Ion.deserialize bytes with | ||
| | .error (off, msg) => throw s!"Error reading Ion: {msg} (offset = {off})" | ||
| | .ok a => | ||
| if p : a.size = 1 then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you use the p hypothesis anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove p : then the next line with a[0] fails to prove. I don't know how Lean decides to use this fact implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace with _ : a.size = 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not valid syntactically 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note there is a if p : a.size = 1 then where the p is not used somewhere else in the codebase as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, match _ : x with is fine but if _ : x then ... is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not short, but I frequently will handle this with a line like the following in monadic code.
let .isTrue p := inferInstanceAs (Decidable (a.size = 1))
| throw "blah blah blah"
This performs the checks, and subsequent code has access to the proof while the exception path is short.
| throw s!"{name} program found when {dialect} expected." | ||
| fromIonFragment frag dialects dialect | ||
|
|
||
| def filesFromIon (dialects : DialectMap) (bytes : ByteArray) : Except String (List StrataFile) := do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it shouldn't live in the Program namespace. Maybe move up one level and rename fileFromIon back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be StrataFile.filesFromIon and appear below Program things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice I have 0 intuition for what should be in what namespace. filesFromIon parses Ion into programs (nested in StrataFile), so why shouldn't it live in the Program namespace?
Maybe this could be StrataFile.filesFromIon
When do you decide to put things in a namespace? Can you give me any guidelines for how to decide to put things in a particular file or in a particular namespace?
Without understanding the context very well, my inclination would be to have a single namespace per file, so you wouldn't be able to have multiple definitions with the same name in a single file, but that seems fine to me. Why not put everything in this file in a Strata.DDM.Ion namespace?
In any case, right now I'm happy to name/move things in whatever way you prefer, but it wasn't entirely clear to me what you meant with "appear below Program things". If you're willing to move/name the things yourself, that could help speed-up the PR, since then I can approve your commit and @aqjune-aws can be the second approver and code-owner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, I mostly keep things in the same top-level namespace except for three reasons:
- I can use dot notation to access it (e.g., I'd put an operation like
count (p : Program) : NatinProgramso I can writep.count. - It is closely coupled to a particular type and has a generic name (e.g.,
Program.fromIon). Instances follow this pattern even if their names are typically auto-generated. - If it is closely coupled to a method following pattern (1) and (2), and not intended for much use. e.g., an auxillary function needed to implementation some other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of slow at task switching and want to prioritize my regular work commitments.
I think I am fine with leaving this as is also; it doesn't matter that much if I don't see it as idiomatic.
307f9a7 to
79ee155
Compare
Changes
.leaninto a.stfile, so it can be consumed by thejavaGencommandfnortype, since those are not needed and are not supported byjavaGenlaurelAnalyzecommand to StrataMain that for now will serve as a root for Laurel related interactions. I can imagine that in the future we will introduce an analyze command that is agnostic to the dialect used.Testing
Ion.fromIonFiles