execute command line initial implementation #241

iisaduan · 2025-01-21T08:59:44Z

tsc can be invoked by writing tsc as the second position, for example

tsgo tsc [rest of args to be passed into tsc]

Without tsc in the second position, there is no change to tsgo flag parsing/execution

ToDos for after this PR:

Things that are not yet implemented:
- Watch, build, incremental, and display functions like showConfig, help, version, are not implemented
- Not all error handling/outputs/printing are implemented
Possible places for refactors:
- System vs Host vs Program layering: discussion, comment System vs Host vs Program layering #272
- May need to revisit path handling for tsconfig/project references: discussion
Testing
- most tests are not yet ported
- environment variables do not do anything
- refactor testsys to get rid of `System.EndWrite()": discussion 1, discussion 2
- nilpointer crash: extends nilpointer crash in executeCommandLine #271

internal/execute/system.go

cmd/tsgo/main.go

internal/execute/system.go

jakebailey · 2025-01-22T19:49:50Z

internal/execute/system.go

+}
+
+func (s *osSys) EndWrite() {
+	// do nothing, this is needed in the interface for testing


I'm curious, when is this called? Is it a flushing operation? Something else?

Every occurrence of "write" is tracked in the original code for several reasons, and the tracking is most used probably with screen clears (which seem mostly relevant in watch?) The reason why this is needed is because the old code implemented write to be a separate entry each time we called write. However, because we are using stringwriters in diagnostic outputting, and we could be passing the writers through fmt.fprint.., several times in each diagnostic writing function, without an endwrite(), writing in the test system generates different output based on if it's implemented as

fmt.fprint(sys, "Output::") fmt.fprint(sys, errors) fmt.fprint(sys, "\n")

vs

fmt.fprint(sys, "Output::" + errors + "\n")

and i don't see an easier way to make them the same atm. I hope to be able to simplify it eventually, but I don't want to make too many changes when watch isn't implemented yet, since that's the part where "each write" is tracked.

That makes sense, though it seems like the assertion doesn't have to be per write, but it could be the entire output string buffered, right?

But, I can understand not wanting to mess with it for a port.

I'll continue to take a look at your buffer suggestion as I port/see what testing looks like after i finish adding tsconfig parsing to tests right now

I would like this to be a TODO for a later PR since I would like testing to not be the priority for this PR

internal/execute/system.go

jakebailey · 2025-01-22T19:51:15Z

internal/execute/verifytsc/testsys.go

+	// todo: revisit if improving tsc/build/watch unittest baselines
+	s.output = append(s.output, s.currentWrite.String())


Couldn't we just write all output to a bytes.Buffer or something, and then check it there? Checking line-based output seems like a lot of work.

#241 (comment)

…o executeCommandLine

internal/execute/verifytsc/verifytsc.go

cmd/tsgo/main.go

cmd/tsgo/sys.go

jakebailey · 2025-01-28T19:09:06Z

internal/compiler/host.go

@@ -11,6 +11,7 @@ import (

 type CompilerHost interface {
 	FS() vfs.FS
+	SetOptions(options *core.CompilerOptions)


I suspect this isn't required; seems odd for the host to have a setter when that's the opposite direction of the flow of data...

(I think you have a comment of the sort in another file now that I read)

Hm, but the host does acutally have it for other things. Makes me think the existing code isn't quite right.

Makes me think that the existing code isn't quite right

Which existing code? I often had a hard time deciding which things should belong to which interface, so it's pretty possible that I'm just confused, and also possible that the implementer of existing code also wasn't sure how things would shake out in the future/also was confused.

I mean the existing code implements hosts with methods that depends on options, which implies the host can answer questions about a config that hasn't even been set yet.

oh yeah, it hits a lot of nil

Eventually for things like watch, the options would need to propagate back up to the host. It seems to make enough sense to me?

That feels a bit like a recipe for weird races / behavior, or confusing behavior if we want to share the host between multiple Programs... If we're saying this thing is created by Program for it to use, then I'm less afraid of it, but the way I'm reading it right now is that we are constructing one with NewCompilerHost to give to a Program.

For watch mode, I would expect us to take one program, then use it to construct another program, which would involve giving it the last program's info, not reusing the same host object with mutable state...

I see what you mean. I think the confusion comes from strada having compilerOptions be a one-and-all object to hold everything, and at each level, not every option is used. For example, incremental is only used in executeCommandLine and emitter, and I'm pretty sure that the separate watchOptions object gets combined into the compilerOptions object at some points, but because the watchoptions aren't used in places like checker, we don't really see that.

So the CompilerHost would only need a subset of compilerOptions, and Program would need some overlapping subset. Since my implementation here has the parsed options being passed into the Program creation, that part for Program should be fine? 🤔

On the other hand, it might be wrong to reset CompilerHost.options during program creation 🤔, but I'm not sure that at this state we could tell, since we're only doing the config parsing once

(As far as I can tell, if I comment that line out, there's no difference in behavior atm)

internal/compiler/program.go

jakebailey · 2025-01-28T19:10:39Z

internal/compiler/program.go

+	// todo: remove/edit setOptions call if compilerHost no longer has CompilerOptions. Since compilerHost was created before options were parsed, compiler host would
+	//     not have options yet


Yeah, this sort of split feels weird.

…o executeCommandLine

iisaduan

@navya9singh generally, I'm not sure if I'm missing places if I'm setting/saving the parsed compiler options or doing path handling for tsconfigparsing correctlu

internal/core/compileroptions.go

internal/execute/tsc.go

internal/tsoptions/commandlineparser.go

internal/tsoptions/parsinghelpers.go

internal/tsoptions/tsconfigparsing.go

internal/execute/tsc.go

iisaduan · 2025-01-29T23:32:29Z

internal/execute/tsc.go

+	tsConfigSourceFile := tsoptions.NewTsconfigSourceFileFromFilePath(configFileName, configFileText)
+	cwd := sys.Host().GetCurrentDirectory()
+	tsConfigSourceFile.SourceFile.SetPath(tspath.ToPath(configFileName, cwd, sys.FS().UseCaseSensitiveFileNames()))
+	// tsConfigSourceFile.resolvedPath = tsConfigSourceFile.FileName()


@navya9singh are these fields are needed?

I believe they are when files are included through project references. @jakebailey ?

I don't really know off the top of my head, but that sounds plausible.

gabritto

I mostly only had questions, nothing looked too weird to me that wasn't weird in TS already.

internal/execute/system.go

gabritto · 2025-01-30T00:05:37Z

internal/execute/tsc_test.go

+	}).verify(t)
+}
+
+// func TestExtends(t *testing.T) {


Might be good to add a todo comment here saying we can run those tests when we have support for extends.

I thought we already supported extends? vscode and the compiler are working.

These tests run into nilpointer errors. I think it may be an issue with path handling that wasn't evident in the tsconfigparsing tests for extends

Right, but extends doesn't have the exception when run via tsgo, so it feels odd for it to fail...

On the same test cases?

No, just on vscode and src/compiler; obviously not representative but I was just hoping this could run with some skips on specific tests or something. It's not a big deal, when I left my comment first I thought this was just accidentally commented out from before we supported it.

gabritto · 2025-01-30T00:36:18Z

internal/compiler/program.go

-func (p *Program) GetOptionsDiagnostics() []*ast.Diagnostic {
-	return p.optionsDiagnostics
-}
+func (p *Program) SourceFiles() []*ast.SourceFile              { return p.files }


Not about this PR, but: why does compiler host have compiler options in the first place?

I see there's a discussion on this elsewhere. I also agree it's weird.

internal/compiler/program.go

internal/execute/testsys_test.go

cmd/tsgo/sys.go

jakebailey · 2025-01-30T04:43:29Z

internal/execute/tsc_test.go

+	}).verify(t)
+}
+
+// func TestExtends(t *testing.T) {


I thought we already supported extends? vscode and the compiler are working.

internal/execute/tsc.go

internal/tsoptions/commandlineparser.go

internal/execute/tsc.go

navya9singh · 2025-01-30T22:05:28Z

internal/execute/tsc.go

+	tsConfigSourceFile := tsoptions.NewTsconfigSourceFileFromFilePath(configFileName, configFileText)
+	cwd := sys.Host().GetCurrentDirectory()
+	tsConfigSourceFile.SourceFile.SetPath(tspath.ToPath(configFileName, cwd, sys.FS().UseCaseSensitiveFileNames()))
+	// tsConfigSourceFile.resolvedPath = tsConfigSourceFile.FileName()


I believe they are when files are included through project references. @jakebailey ?

internal/execute/tsc.go

internal/tsoptions/parsinghelpers.go

cmd/tsgo/main.go

Co-authored-by: Jake Bailey <[email protected]>

…o executeCommandLine

internal/execute/tsc.go

jakebailey

Just nits.

cmd/tsgo/sys.go

internal/execute/outputs.go

Co-authored-by: Jake Bailey <[email protected]>

iisaduan added 2 commits January 21, 2025 00:50

execute command line

2fd091c

enable emit

692c3bb

iisaduan requested a review from jakebailey January 21, 2025 08:59

jakebailey reviewed Jan 22, 2025

View reviewed changes

iisaduan added 6 commits January 22, 2025 14:56

Merge branch 'main' of https://github.com/microsoft/typescript-go int…

b26dc18

…o executeCommandLine

move sys to main.go

175aebf

Merge branch 'main' of https://github.com/microsoft/typescript-go int…

eac308b

…o executeCommandLine

add tsconfigparsing

2141369

add file baselining

c31d18d

Merge branch 'main' of https://github.com/microsoft/typescript-go int…

c258b28

…o executeCommandLine

jakebailey reviewed Jan 28, 2025

View reviewed changes

internal/execute/verifytsc/verifytsc.go Outdated Show resolved Hide resolved

jakebailey reviewed Jan 28, 2025

View reviewed changes

iisaduan added 8 commits January 28, 2025 13:26

move tests into execute_test

3984ee0

move os.exit into main only

3003b5c

clean up, check tsconfig parsing

a8924da

Write() -> Writer() and returns a io.writer

609e180

remove sys.exit()

42fe8e4

converttooptionswithabsolutepaths

bb688b5

Merge branch 'main' of https://github.com/microsoft/typescript-go int…

4b0a00e

…o executeCommandLine

more code clean up

62fd055

iisaduan commented Jan 29, 2025

View reviewed changes

refactor host to be created much later (and other clean ups)

399b4d7

gabritto approved these changes Jan 30, 2025

View reviewed changes

jakebailey reviewed Jan 30, 2025

View reviewed changes

iisaduan added 2 commits January 30, 2025 13:19

remove diagnosticreporter

0b89c7a

unexport sys things

2b99f18

navya9singh reviewed Jan 30, 2025

View reviewed changes

address comments, fix tests

098e82d

jakebailey reviewed Jan 31, 2025

View reviewed changes

cmd/tsgo/main.go Outdated Show resolved Hide resolved

iisaduan commented Jan 31, 2025

View reviewed changes

cmd/tsgo/main.go Outdated Show resolved Hide resolved

iisaduan and others added 2 commits January 30, 2025 17:26

refactor how tsc arg is checked in main.go

8045b91

Co-authored-by: Jake Bailey <[email protected]>

Merge branch 'main' of https://github.com/microsoft/typescript-go int…

4d12fb7

…o executeCommandLine

jakebailey reviewed Feb 3, 2025

View reviewed changes

internal/execute/tsc.go Outdated Show resolved Hide resolved

iisaduan added 3 commits February 3, 2025 14:35

refactor format opts

085905a

refactor pretty

8a45430

move testing function

227e3a0

jakebailey approved these changes Feb 3, 2025

View reviewed changes

cmd/tsgo/sys.go Outdated Show resolved Hide resolved

cmd/tsgo/sys.go Outdated Show resolved Hide resolved

cmd/tsgo/sys.go Outdated Show resolved Hide resolved

internal/execute/outputs.go Show resolved Hide resolved

newline -> newLine

0560997

Co-authored-by: Jake Bailey <[email protected]>

iisaduan merged commit 2cf4cb4 into microsoft:main Feb 3, 2025
14 checks passed

iisaduan mentioned this pull request Feb 3, 2025

System vs Host vs Program layering #272

Open

iisaduan deleted the executeCommandLine branch February 3, 2025 23:45

jakebailey mentioned this pull request Feb 12, 2025

Fix diagnostic handling, unbreaking bind timing #346

Merged

		// todo: revisit if improving tsc/build/watch unittest baselines
		s.output = append(s.output, s.currentWrite.String())

		// todo: remove/edit setOptions call if compilerHost no longer has CompilerOptions. Since compilerHost was created before options were parsed, compiler host would
		// not have options yet

execute command line initial implementation #241

execute command line initial implementation #241

Uh oh!

Conversation

iisaduan commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iisaduan commented Jan 21, 2025 •

edited

Loading

iisaduan Jan 22, 2025 •

edited

Loading

iisaduan Jan 23, 2025 •

edited

Loading

iisaduan Jan 29, 2025 •

edited

Loading

iisaduan Jan 29, 2025 •

edited

Loading

iisaduan Jan 29, 2025 •

edited

Loading