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

Expose true filename to rules #11989

Closed
Conduitry opened this issue Jul 15, 2019 · 50 comments · Fixed by #14616
Closed

Expose true filename to rules #11989

Conduitry opened this issue Jul 15, 2019 · 50 comments · Fixed by #14616
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint good first issue Good for people who haven't worked on ESLint before
Projects

Comments

@Conduitry
Copy link

Conduitry commented Jul 15, 2019

The version of ESLint you are using.

6.0.1

The problem you want to solve.

This. Some rules, like eslint-plugin-import's no-unresolved, need to know the true path of the current file, not a path that's been potentially assembled using a processor's named code block.

Your take on the correct solution to problem.

I'm not entirely sure, but perhaps a new option to context.getFilename() to return the true path rather than the one potentially created from a processor's named code blocks.

Are you willing to submit a pull request to implement this change?

Yes.

@Conduitry Conduitry added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jul 15, 2019
@platinumazure
Copy link
Member

platinumazure commented Jul 15, 2019

@mysticatea @not-an-aardvark @nzakas Any thoughts on this use case?

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 15, 2019

Could you elaborate on how the virtual filenames cause problems? I read import-js/eslint-plugin-import#1415 but it's not clear to me why relative paths would be broken, since it seems like resolving paths relative to the virtual filename would have the same result as resolving relative to the real filename (since the virtual filename has the same directory).

@Conduitry
Copy link
Author

Conduitry commented Jul 15, 2019

The virtual filename is down one level from the real filename -

const blockName = path.join(filename, `${i}_${block.filename}`);

@platinumazure
Copy link
Member

platinumazure commented Jul 15, 2019

@not-an-aardvark Is that the case? My understanding was that for a physical file (say, path/to/foo.html), the virtual files were added on to the full file path (say, path/to/foo.html/0.js). But maybe I'm mistaken.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 15, 2019

Oh I see, thanks for clarifying.

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 15, 2019
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jul 15, 2019

An alternative fix might be to not alter the implied cwd for named block filenames, but instead use a query string or an extension or something?

@mysticatea
Copy link
Member

mysticatea commented Jul 15, 2019

Sounds reasonable request.

@Conduitry
Copy link
Author

Conduitry commented Aug 5, 2019

What's next here?

I don't know how much of the way the virtual preprocessed block's filename is constructed is part of the API contract. Using a query string or appending some other string that doesn't contain a forward slash would be tidier (and would probably remove the need for an option to control which filename we wanted), but it does seem to contradict what's in the docs: https://eslint.org/docs/user-guide/configuring#specifying-processor

Something to consider. I don't feel qualified to have an opinion about this. But I do think that whatever is decided to be done here, the docs should be made to be more explicit about how the filename is constructed. The current /[index]_[name].[extension] isn't mentioned at all. It may or may not be too late to also take this opportunity to change it. I don't know how well-used named blocks are.

@mysticatea
Copy link
Member

mysticatea commented Aug 23, 2019

I'm sorry for my late response.

As a fact, context.getFilename() doesn't guarantee to return a real path. For example, people can give an arbitrary string as a filename.

const { CLIEngine } = require("eslint");
const engine = new CLIEngine();

engine.executeOnText("console.log('hello')", "a-dummy-filename.js")

In that case, ESLint doesn't check if the file path is valid or not.

Also, eslint command supports piping and context.getFilename() returns "<input>" if piped.

Additionally, processors may make dummy filenames for each code block. It must be a path that people can distinguish between code blocks and files by glob patterns because people often use different settings for code blocks. By design, it's like the child file of the original file. (for example, yes, https://eslint.org/docs/user-guide/configuring#specifying-processor.)


I feel that this is related to #12133. If we added context.getRealFilename(), should it guarantee that the returned path exists as a file?

@ljharb
Copy link
Sponsor Contributor

ljharb commented Aug 23, 2019

You say “by design”, but to me that seems like a broken design, because making it be in a subdir changes relative paths. It should be a sibling, so that relative paths are still correct.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Sep 23, 2019
@eslint-deprecated
Copy link

eslint-deprecated bot commented Sep 23, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 23, 2019

This should be reopened; it affects the plugin ecosystem.

@g-plane g-plane reopened this Sep 23, 2019
@g-plane g-plane removed the auto closed The bot closed this issue label Sep 23, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Oct 24, 2019
@eslint-deprecated
Copy link

eslint-deprecated bot commented Oct 24, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 24, 2019

Ping again; this should remain open because it affects the plugin ecosystem.

@g-plane g-plane reopened this Oct 25, 2019
@eslint-deprecated
Copy link

eslint-deprecated bot commented Nov 25, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@btmills
Copy link
Member

btmills commented Apr 21, 2021

If we're unable to change the filename string, our remaining options are, in descending order of personal preference:

  1. Add context.filename and context.codeBlock. In the case of nested code blocks, would codeBlock continue to use / as the separator?
  2. Instead of adding an arg, add a new getPhysicalFilename() method for this use case.
  3. Add an argument to getFilename(). I dislike Boolean args as well, but an options object feels like overkill.
  4. Change getFilename() to always return the physical filename, and add a new getVirtualFilename() method that returns the virtual filename in the case of a code block. This would be a breaking change.

That's everything that I saw suggested already plus a couple variations. Are there any other ideas?

Unless we go with the fourth option, this is no longer a breaking change and can be removed from the v8.0.0 project to be solved in any minor release.

@nzakas
Copy link
Member

nzakas commented Apr 22, 2021

We can’t do option 4 because some rules rely on the return value to be unique (this is why we changed it to return filename plus code block in the first place).

Options 1 or 2 seem the most viable. Option 1 feels cleaner and would seem to better match expectations but I do worry that people would forget to check codeBlock when using the filename as a cache key. So, I think I’ve talked myself into option 2.

Also, removing from v8.0.0 since none of these options are breaking.

@nzakas nzakas removed this from Planned in v8.0.0 Apr 22, 2021
@btmills
Copy link
Member

btmills commented Apr 22, 2021

Option 2, getPhysicalFilename(), works for me.

@nzakas nzakas removed the needs design Important details about this change need to be discussed label Apr 23, 2021
@nzakas
Copy link
Member

nzakas commented Apr 23, 2021

Summary: we will add a new getPhysicalFilename() method to the rule context object. This method will return the full path of the file on disk without any code block information. This is simple enough that we don’t need an RFC.

@nzakas nzakas moved this from Evaluating to Ready to Implement in Triage Apr 23, 2021
@nzakas nzakas added the good first issue Good for people who haven't worked on ESLint before label Apr 23, 2021
@snitin315
Copy link
Contributor

snitin315 commented Apr 23, 2021

Can I work on this issue?

@nzakas
Copy link
Member

nzakas commented Apr 24, 2021

Let’s wait a week to see if any first-time contributors would like to take this on. If no one else steps up, then it’s all yours.

@harsha0609
Copy link

harsha0609 commented Apr 24, 2021

Hi @nzakas, I would like to volunteer to work on this issue.

@nzakas
Copy link
Member

nzakas commented Apr 26, 2021

@harsha0609 awesome! We'd love to have you work on this issue. Please let us know if you need any help.

@pmcelhaney
Copy link
Contributor

pmcelhaney commented May 1, 2021

Until the issue is fixed, here's a work-around to disable affected rules for these bogus filenames.

       {
            overrides: [
                { // Can remove after this issue is fixed: https://github.com/eslint/eslint/issues/11989
                    files: ['**/*.md/*_*.js'],
                    rules: {
                        'import/no-unresolved': 'off',
                    },
                },
       }

Note that I'm specifically concerned about the import/no-unresolved rule in Markdown (.md) files. You may need to tweak to suit your needs.

@nzakas
Copy link
Member

nzakas commented May 3, 2021

@harsha0609 just checking in to see if you're still planning on working on this? No worries if not, we just want to give you the time to dig in if you want.

@harsha0609
Copy link

harsha0609 commented May 3, 2021

Sorry for the delay @nzakas, was caught up with some work stuff, so yeah I still would love to work on this, could you please help me out with the rule context object definition for this?
As in where I can find it.

Summary: we will add a new getPhysicalFilename() method to the rule context object. This method will return the full path of the file on disk without any code block information. This is simple enough that we don’t need an RFC.

@pmcelhaney
Copy link
Contributor

pmcelhaney commented May 3, 2021

Hi @harsha0609, looks like the object is defined here:

getFilename: () => filename,

You'll want to add a getPhysicalFilename() method to that object. If you search for getFilename() you can also find the places where the tests and documentation will need to be updated.

@nzakas
Copy link
Member

nzakas commented May 4, 2021

Thanks @pmcelhaney!

@harsha0609 you’ll also likely need to pass another argument to this function:

function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parserName, settings, filename, disableFixes, cwd) {

@harsha0609
Copy link

harsha0609 commented May 4, 2021

@nzakas went through the code, guess won't be able to work on this. Sorry for the inconvenience.

@pmcelhaney
Copy link
Contributor

pmcelhaney commented May 4, 2021

I'll wait to see if anyone else wants to claim it, but I have a pretty good idea how to move forward, so I'll go ahead and share my thoughts.

There's an options object where the filename is replaced by "blockName".

eslint/lib/linter/linter.js

Lines 1303 to 1326 in 41b3570

const blockName = path.join(filename, `${i}_${block.filename}`);
// Skip this block if filtered.
if (!filterCodeBlock(blockName, blockText)) {
debug("This code block was skipped.");
return [];
}
// Resolve configuration again if the file content or extension was changed.
if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) {
debug("Resolving configuration again because the file content or extension was changed.");
return this._verifyWithConfigArray(
blockText,
configForRecursive,
{ ...options, filename: blockName }
);
}
// Does lint.
return this._verifyWithoutProcessors(
blockText,
config,
{ ...options, filename: blockName }
);

We would need to put a copy of the original filename in the options object in a key called physicalFilename.

Then pass options.physicalFilename in a new argument to the function @nzakas mentioned.

@nzakas
Copy link
Member

nzakas commented May 5, 2021

That looks like a good approach 👍

@snitin315
Copy link
Contributor

snitin315 commented May 19, 2021

Hi @pmcelhaney, are you still working on it? If not I would like to work on this one.

@pmcelhaney
Copy link
Contributor

pmcelhaney commented May 22, 2021

@snitin315 No, I'm not working on it. Thank you for picking it up!

@nzakas nzakas moved this from Ready to Implement to Pull Request Opened in Triage May 29, 2021
Triage automation moved this from Pull Request Opened to Complete Jun 4, 2021
mdjermanovic added a commit that referenced this issue Jun 4, 2021
#14616)

* New: add `getPhysicalFilename()` method to the rule context object

* Docs: update

* Chore: add test

* Chore: update more instances

* Chore: apply suggestions

* Chore: apply suggestions

* Chore: fix typo

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 2, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint good first issue Good for people who haven't worked on ESLint before
Projects
Triage
Complete