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

[display] an option to read stdin to get the content of file specified in --display #4651

Closed
nadako opened this issue Nov 18, 2015 · 23 comments
Assignees
Labels
feature-ide IDE / Editor support test-disabled
Milestone

Comments

@nadako
Copy link
Member

nadako commented Nov 18, 2015

Subj. This would be REALLY useful for editors since they work with unsaved files a lot and having to use temp file for auto-completion is quite stupid and error-prone.

(btw, I remembered about this when playing with VS Code api: https://github.com/nadako/vscode-haxe)

@nadako nadako added the feature-ide IDE / Editor support label Nov 18, 2015
@nadako
Copy link
Member Author

nadako commented Nov 19, 2015

Unless I'm missing something, this should be quite easy to implement:

  • In Typeload.parse_file, if some option (a define, maybe?) is specified, we check whether given file path is the same as Parser.resume_display.pfile and read stdin instead of opening file channel.
  • In completion server's parse_hook, if that option is specified, we skip caches for the display file, so it always goes to Typeload.parse_file which does the stdin reading.

cc @ncannasse

@Simn
Copy link
Member

Simn commented Nov 19, 2015

From my experience with stdin it always seems easy to implement but never actually is. I once tried to get it working for --eval but ran into some issues I don't remember.

@nadako
Copy link
Member Author

nadako commented Nov 19, 2015

I wonder what exact troubles you ran into. I'm looking at Go's completion server docs right now (https://github.com/nsf/gocode/blob/master/docs/IDE_integration.md) - they support reading completion file from stdin and this is actually used by most of Go plugins.

@nadako
Copy link
Member Author

nadako commented Nov 19, 2015

Okay, one issue is that completion server should not read from stdin, but from the socket somehow.

@ncannasse
Copy link
Member

TBH that's quite complex versus having the editor save the file before getting completion.

@ncannasse
Copy link
Member

PS: if you have several open files, you need to save them anyway

@ruby0x1
Copy link

ruby0x1 commented Nov 24, 2015

This is a really big deal actually.

Having implemented both atom and sublime plugins 99% of the problem comes from the fact that the file has to be persisted to disk.

Just a few of the real problems this runs into:

  • saving the file modifies the disk file
  • editors notice changes to the file, then try or automatically reload it (sometimes losing undo history, etc)
  • saving the file doesn't reliably use utf-8 encoding so the byte positions don't line up with what haxe expects
  • force saving in utf-8 to avoid this, may mean a different encoding, and then has to reverse that, which is more error prone (remember this is peoples actual code/work we are risking loss against)
  • asynchronous saving will 100% trip up the automatic reload behaviours of editors and much more, which means that the operations have to be blocking to be reliable

That's just the editor behaviours,
The solution most have landed on is to save the temp file and restore from it (so the user content is unchanged) but this is disk-speed operations that stack, adding to completion times.

It has also been solved in some form once by completing against a temp file, not the real file itself. However in this case, it has to be persisted to disk with an exact matching classpath relative to the build context and cwd when calling haxe. The surrounding structure must match, otherwise it will just fall apart.

I've dreamed about just calling --display with source or bytes, stdin, or being able explicitly tell the compiler the path, but specifying another path for it to read. like: --display /tmp/some/tmpfile.hx;the/real/file.hx@10.

This would solve A LOT by allowing us to say which file we are completing against but a different path to read the source content from.

The best case for speed and usability would definitely be stdin (or arg based) though.

I strongly urge you to consider that completion (plus associated features) and strong tooling is a crucial part of the haxe ecosystem, having it not be the best it possibly can because of lack of perceived relevance will be sad.

The newer completion features and improvements have been taking great strides, but to me this is (from the trenches) a higher priority than any of those newer features going forward.

@nadako
Copy link
Member Author

nadako commented Nov 24, 2015

I experienced exactly the same problems trying to implement a plugin for ST.

@jeremyfa
Copy link

This is crucial.

It should never be required to touch the original file for completion (saving a file should only be triggered by the user explicitly).

Best option would be to provide the contents through stdin (or arg based), but being able to specify another file path for temporary content would already be a huge improvement.

I also agree that this is way more important at the moment than adding more completion features.

+1

@ncannasse
Copy link
Member

Just a question for those that had this problem : were you not able to trigger the editor "Save" or "SaveAll" functionality? This is what I did when I first created the Haxe plugin for FlashDevelop and I never had such issues.

@ncannasse
Copy link
Member

@jeremyfa simply saving the files in a temp dir then adding a -cp at the end should work already

@ncannasse
Copy link
Member

The problems for implementing "file injection" are the following:

  • we cannot rely on file timestamp for compilation cache
  • we cannot send the file in args since commandline args size are maxed
  • we cannot use stdin and need socket support and thus a complete protocol for compilation server
  • this would require to implement some kind of abstract file system at the very low level to ensure that all files operations are implemented whatever the file comes from

@jeremyfa
Copy link

@ncannasse Having tried in many different ways the temp dir with a -cp, I did not succeed to have consistent behavior if I -cp the same file in a temporary directory.

For instance, if I want code completion for some.module.Name.hx, duplicating the file into some /tmp/dir/src/some/module/Name.hx and adding the related -cp /tmp/dir (tried with -cp /tmp/dir/src as well), will sometimes work, sometimes fail.

Suspecting that these inconsistencies are due to having a duplicate file describing the same class path, I tried another solution:

  • Get the contents of some.module.Name.hx
  • Detect the package some.module; line in the contents and replace it with package some.module.__tmp_package;
  • Save these contents in /tmp/dir/src/some/module/__tmp_package/Name.hx and add the related -cp

Because this is actually another file describing another class path, the conflict doesn't exist anymore and I can get consistently code completion. However, in addition to the hacks above (that require parsing the content, changing it and adding additional fake files), I have to remove the references to __tmp_package from the output before using it.

This kinda works (implemented in the atom package and activable from the settings) because the sub-package has access to its parent. But I believe it's not right. I wished there was another way to get code completion without touching the file.

@ruby0x1
Copy link

ruby0x1 commented Nov 25, 2015

  • we cannot rely on file timestamp for compilation cache
    • just for the file being completed?
  • we cannot send the file in args since commandline args size are maxed
    • fair enough
  • we cannot use stdin and need socket support and thus a complete protocol for compilation server
    • isn't it already a socket and already doing some of this?
  • this would require to implement some kind of abstract file system at the very low level to ensure that all files operations are implemented whatever the file comes from
    • I don't understand why it would need this. The described idea just lies about which file path it is reading for completion.

But the problems you picture of handling the completion : Those problems are currently shifted onto the user space, but they aren't carrying the benefit of the entire haxe typing, parsing, context ready to answer any question about the code at their disposal.

Maybe the exact ideas don't fit but the reality is unchanged, dealing with the code completion server as it stands due to the issues mentioned has an imbalance, which is putting the ecosystem and haxe users at a disadvantage.

I'll try see what other existing solutions are being used in things like Go.

@jcward
Copy link
Contributor

jcward commented Dec 18, 2015

Ha, I just ran into this very issue poking at @nadako's vscode plugin. I haven't gotten far, but I have already seen an error message, something like "save failed: contents of file are newer than buffer." It doesn't happen all the time, though.

@jcward
Copy link
Contributor

jcward commented Dec 19, 2015

FYI, here's the error I get occasionally while typing in VSCode when it saves the file often:

image

I suppose I'll try a tmp -cp as Nicolas suggested.

@waneck
Copy link
Member

waneck commented Dec 21, 2015

I like that! Also we could probably do some optimizations by caching the
file tree while a compilation is being run - IIRC Haxe access the
filesystem quite a lot while looking for the basic types like Stirng

2015-12-21 17:48 GMT-02:00 Jeff Ward [email protected]:

Wow, looks like @pleclech https://github.com/pleclech has taken a stab
at it:

https://twitter.com/pleclech/status/679022509694558208
https://github.com/pleclech/haxe/tree/memory-file
https://github.com/pleclech/vscode-haxe/tree/using-memory-patcher


Reply to this email directly or view it on GitHub
#4651 (comment)
.

@ousado
Copy link
Contributor

ousado commented Mar 13, 2016

Just FYI for vscode-haxe users: thanks to @pleclech, users can set a temporary directory in File > Preferences > Workspace Settings, by overriding the "haxe.haxeTmpDirectory": "" setting, which will then be used for completion.

@nadako
Copy link
Member Author

nadako commented Apr 16, 2016

Hey guys, could you please try this as described in #5120 and see if it works for you?

@ruby0x1
Copy link

ruby0x1 commented Apr 16, 2016

Will do, this weekend! cc @jeremyfa

@nadako
Copy link
Member Author

nadako commented Apr 16, 2016

Thanks!

@nadako nadako assigned nadako and unassigned ncannasse Apr 16, 2016
@nadako nadako modified the milestones: 3.3.0-rc1, 3.4 Apr 16, 2016
@jeremyfa
Copy link

A bit delayed, but I confirm display-stdin in combination with server's --wait stdio works great on my side! (with atom, achieved mostly by looking at the languageserver experiment code)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-ide IDE / Editor support test-disabled
Projects
None yet
Development

No branches or pull requests

9 participants